| Message ID | 1448429044-10395-1-git-send-email-jaswinder.singh@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/25/2015 06:24 AM, Jassi Brar wrote: > The GT counter is common to every core in a cluster. There is a usecase > when Linux is spawned by a 'master' firmware/OS running on some core of > the same cluster and the GT is used by the both. > Linux, upon boot, resetting the GT counter is obviously fatal to the > other OS. So provide a way for DT to tell Linux if it's running in that > 'slave' mode and must not reset the counter. > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> Hmm, I doubt that is a hardware description. Rob ? One comment below assuming this is acceptable. > --- > Documentation/devicetree/bindings/arm/global_timer.txt | 6 ++++++ > drivers/clocksource/arm_global_timer.c | 12 +++++++++--- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt > index bdae3a8..bb897a9 100644 > --- a/Documentation/devicetree/bindings/arm/global_timer.txt > +++ b/Documentation/devicetree/bindings/arm/global_timer.txt > @@ -17,6 +17,12 @@ > > - clocks : Should be phandle to a clock. > > + > +** Optional properties: > + > +- arm,gt_no_reset : Firmware/bootloader already initialized the > + global timer-counter and expects it to be not reset again. > + > Example: > > timer@2c000600 { > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index a2cb6fa..952bab6 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -51,6 +51,7 @@ static void __iomem *gt_base; > static unsigned long gt_clk_rate; > static int gt_ppi; > static struct clock_event_device __percpu *gt_evt; > +static bool gt_reset_counter; Defining a global static boolean just for a check in the init function is ... pointless. Adding a parameter to clocksource_init would make more sense. > /* > * To get the value from the Global Timer Counter register proceed as follows: > @@ -212,9 +213,11 @@ static u64 notrace gt_sched_clock_read(void) > > static void __init gt_clocksource_init(void) > { > - writel(0, gt_base + GT_CONTROL); > - writel(0, gt_base + GT_COUNTER0); > - writel(0, gt_base + GT_COUNTER1); > + if (gt_reset_counter) { > + writel(0, gt_base + GT_CONTROL); > + writel(0, gt_base + GT_COUNTER0); > + writel(0, gt_base + GT_COUNTER1); > + } > /* enables timer on all the cores */ > writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); > > @@ -303,6 +306,9 @@ static void __init global_timer_of_register(struct device_node *np) > goto out_irq; > } > > + /* See if we are told we can't reset the global timer counter */ > + gt_reset_counter = !of_property_read_bool(np, "arm,gt_no_reset"); > + > /* Immediately configure the timer on the boot CPU */ > gt_clocksource_init(); > gt_clockevents_init(this_cpu_ptr(gt_evt)); >
On Wed, Nov 25, 2015 at 10:54:04AM +0530, Jassi Brar wrote: > The GT counter is common to every core in a cluster. There is a usecase > when Linux is spawned by a 'master' firmware/OS running on some core of > the same cluster and the GT is used by the both. > Linux, upon boot, resetting the GT counter is obviously fatal to the > other OS. So provide a way for DT to tell Linux if it's running in that > 'slave' mode and must not reset the counter. Is the other OS only using the counter? Or are there other porions of the global timer that it assumes its in sole control of? > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > --- > Documentation/devicetree/bindings/arm/global_timer.txt | 6 ++++++ > drivers/clocksource/arm_global_timer.c | 12 +++++++++--- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt > index bdae3a8..bb897a9 100644 > --- a/Documentation/devicetree/bindings/arm/global_timer.txt > +++ b/Documentation/devicetree/bindings/arm/global_timer.txt > @@ -17,6 +17,12 @@ > > - clocks : Should be phandle to a clock. > > + > +** Optional properties: > + > +- arm,gt_no_reset : Firmware/bootloader already initialized the > + global timer-counter and expects it to be not reset again. s/_/-/ in property names. No need for the "gt" prefix, this is on tthe glboal timer node. It "no-counter-reset" seems like a better description, though that really depends on how the other OS is using this. Mark.
On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 11/25/2015 06:24 AM, Jassi Brar wrote: >> >> The GT counter is common to every core in a cluster. There is a usecase >> when Linux is spawned by a 'master' firmware/OS running on some core of >> the same cluster and the GT is used by the both. >> Linux, upon boot, resetting the GT counter is obviously fatal to the >> other OS. So provide a way for DT to tell Linux if it's running in that >> 'slave' mode and must not reset the counter. >> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > > > Hmm, I doubt that is a hardware description. > Its a regular 2core-1cluster platform. The master RTOS on cpu0 maintains the lifecycle of Linux on cpu1. 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how to better convey that platform specific constraint. >> diff --git a/drivers/clocksource/arm_global_timer.c >> b/drivers/clocksource/arm_global_timer.c >> index a2cb6fa..952bab6 100644 >> --- a/drivers/clocksource/arm_global_timer.c >> +++ b/drivers/clocksource/arm_global_timer.c >> @@ -51,6 +51,7 @@ static void __iomem *gt_base; >> static unsigned long gt_clk_rate; >> static int gt_ppi; >> static struct clock_event_device __percpu *gt_evt; >> +static bool gt_reset_counter; > > > Defining a global static boolean just for a check in the init function is > ... pointless. Adding a parameter to clocksource_init would make more sense. > Yes, indeed. Thanks.
On Tue, Dec 15, 2015 at 11:44 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Nov 25, 2015 at 10:54:04AM +0530, Jassi Brar wrote: >> The GT counter is common to every core in a cluster. There is a usecase >> when Linux is spawned by a 'master' firmware/OS running on some core of >> the same cluster and the GT is used by the both. >> Linux, upon boot, resetting the GT counter is obviously fatal to the >> other OS. So provide a way for DT to tell Linux if it's running in that >> 'slave' mode and must not reset the counter. > > Is the other OS only using the counter? > I understand only the Global Counter and the (unused) PreScalar are common to cores in a cluster. > Or are there other porions of the global timer that it assumes its in > sole control of? > More important than 'control' is the fact that the other OS boots first. So it can not survive a reset to the global counter. >> + >> +** Optional properties: >> + >> +- arm,gt_no_reset : Firmware/bootloader already initialized the >> + global timer-counter and expects it to be not reset again. > > s/_/-/ in property names. > > No need for the "gt" prefix, this is on tthe glboal timer node. > OK > It "no-counter-reset" seems like a better description, though that > really depends on how the other OS is using this. > OK will change it to "no-counter-reset". Thanks.
On 12/16/2015 05:11 AM, Jassi Brar wrote: > On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 11/25/2015 06:24 AM, Jassi Brar wrote: >>> >>> The GT counter is common to every core in a cluster. There is a usecase >>> when Linux is spawned by a 'master' firmware/OS running on some core of >>> the same cluster and the GT is used by the both. >>> Linux, upon boot, resetting the GT counter is obviously fatal to the >>> other OS. So provide a way for DT to tell Linux if it's running in that >>> 'slave' mode and must not reset the counter. >>> >>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >> >> >> Hmm, I doubt that is a hardware description. >> > Its a regular 2core-1cluster platform. The master RTOS on cpu0 > maintains the lifecycle of Linux on cpu1. > > 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how > to better convey that platform specific constraint. May be with a kernel parameter ?
On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 12/16/2015 05:11 AM, Jassi Brar wrote: >> >> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano >> <daniel.lezcano@linaro.org> wrote: >>> >>> On 11/25/2015 06:24 AM, Jassi Brar wrote: >>>> >>>> >>>> The GT counter is common to every core in a cluster. There is a usecase >>>> when Linux is spawned by a 'master' firmware/OS running on some core of >>>> the same cluster and the GT is used by the both. >>>> Linux, upon boot, resetting the GT counter is obviously fatal to the >>>> other OS. So provide a way for DT to tell Linux if it's running in that >>>> 'slave' mode and must not reset the counter. >>>> >>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >>> >>> >>> >>> Hmm, I doubt that is a hardware description. >>> >> Its a regular 2core-1cluster platform. The master RTOS on cpu0 >> maintains the lifecycle of Linux on cpu1. >> >> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how >> to better convey that platform specific constraint. > > > May be with a kernel parameter ? > The fact, that arch-timer driver does it via DT (see commit 65b5732d241b8), changes anything? :) Thanks.
On 17/12/15 09:37, Jassi Brar wrote: > On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> On 12/16/2015 05:11 AM, Jassi Brar wrote: >>> >>> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> >>>> On 11/25/2015 06:24 AM, Jassi Brar wrote: >>>>> >>>>> >>>>> The GT counter is common to every core in a cluster. There is a usecase >>>>> when Linux is spawned by a 'master' firmware/OS running on some core of >>>>> the same cluster and the GT is used by the both. >>>>> Linux, upon boot, resetting the GT counter is obviously fatal to the >>>>> other OS. So provide a way for DT to tell Linux if it's running in that >>>>> 'slave' mode and must not reset the counter. >>>>> >>>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >>>> >>>> >>>> >>>> Hmm, I doubt that is a hardware description. >>>> >>> Its a regular 2core-1cluster platform. The master RTOS on cpu0 >>> maintains the lifecycle of Linux on cpu1. >>> >>> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how >>> to better convey that platform specific constraint. >> >> >> May be with a kernel parameter ? >> > The fact, that arch-timer driver does it via DT (see commit > 65b5732d241b8), changes anything? :) This is a horrible, platform specific hack. I wish we could have pushed back on this by forcing people to fix their firmware. Why can't you simply get the firmware to *remove* the node from your device tree altogether? Thanks, M.
On 12/17/2015 10:37 AM, Jassi Brar wrote: > On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> On 12/16/2015 05:11 AM, Jassi Brar wrote: >>> >>> On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> >>>> On 11/25/2015 06:24 AM, Jassi Brar wrote: >>>>> >>>>> >>>>> The GT counter is common to every core in a cluster. There is a usecase >>>>> when Linux is spawned by a 'master' firmware/OS running on some core of >>>>> the same cluster and the GT is used by the both. >>>>> Linux, upon boot, resetting the GT counter is obviously fatal to the >>>>> other OS. So provide a way for DT to tell Linux if it's running in that >>>>> 'slave' mode and must not reset the counter. >>>>> >>>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >>>> >>>> >>>> >>>> Hmm, I doubt that is a hardware description. >>>> >>> Its a regular 2core-1cluster platform. The master RTOS on cpu0 >>> maintains the lifecycle of Linux on cpu1. >>> >>> 'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how >>> to better convey that platform specific constraint. >> >> >> May be with a kernel parameter ? >> > The fact, that arch-timer driver does it via DT (see commit > 65b5732d241b8), changes anything? :) It is debatable (cc'ed Rob). I acked it because the patch was on the edge between soft/hw in the description. But now it sounds like that created a precedence and we consider these DT options exceptions as acceptable. I don't know if there is a better way than a kernel parameter but IMO it makes sense to pass this kind of options via the kernel command line as we can extend it easily for more options in the future. Furthermore, I suspect this feature may be needed for other arch/plat. So having a generic option could be useful for future changes. Thomas, do you have an opinion ? Can we consider adding a set of kernel parameters instead of adding DT exceptions time to time ? eg. clocksource.reset = <0/1> clocksource.virtual = <0/1>
On 17 December 2015 at 15:46, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > I don't know if there is a better way than a kernel parameter but IMO it > makes sense to pass this kind of options via the kernel command line as we > can extend it easily for more options in the future. > I agree, for such 'soft' properties, cmdline is a better option. Thanks.
On 17 December 2015 at 15:38, Marc Zyngier <marc.zyngier@arm.com> wrote: ... > Why can't you > simply get the firmware to *remove* the node from your device tree > altogether? > ... but Linux (on cpu1) and the other OS (on cpu0) both _need_ to use the GT. Thanks.
On Thu, Dec 17, 2015 at 11:16:39AM +0100, Daniel Lezcano wrote: > On 12/17/2015 10:37 AM, Jassi Brar wrote: > >On 16 December 2015 at 12:29, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > >>On 12/16/2015 05:11 AM, Jassi Brar wrote: > >>> > >>>On Tue, Dec 15, 2015 at 11:32 PM, Daniel Lezcano > >>><daniel.lezcano@linaro.org> wrote: > >>>> > >>>>On 11/25/2015 06:24 AM, Jassi Brar wrote: > >>>>> > >>>>> > >>>>>The GT counter is common to every core in a cluster. There is a usecase > >>>>>when Linux is spawned by a 'master' firmware/OS running on some core of > >>>>>the same cluster and the GT is used by the both. > >>>>> Linux, upon boot, resetting the GT counter is obviously fatal to the > >>>>>other OS. So provide a way for DT to tell Linux if it's running in that > >>>>>'slave' mode and must not reset the counter. > >>>>> > >>>>>Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > >>>> > >>>> > >>>> > >>>>Hmm, I doubt that is a hardware description. > >>>> > >>>Its a regular 2core-1cluster platform. The master RTOS on cpu0 > >>>maintains the lifecycle of Linux on cpu1. > >>> > >>>'No_Counter_Reset' is not a h/w thing, I agree, but I don't know how > >>>to better convey that platform specific constraint. > >> > >> > >>May be with a kernel parameter ? > >> > >The fact, that arch-timer driver does it via DT (see commit > >65b5732d241b8), changes anything? :) > > It is debatable (cc'ed Rob). > > I acked it because the patch was on the edge between soft/hw in the > description. But now it sounds like that created a precedence and we > consider these DT options exceptions as acceptable. While not strictly a HW property, it is a property of the system, outside of the control of Linux. It's fine to place such things in the DT when required. I don't think it makes sense to have to use kernel commadn line parameters for this. > I don't know if there is a better way than a kernel parameter but > IMO it makes sense to pass this kind of options via the kernel > command line as we can extend it easily for more options in the > future. > > Furthermore, I suspect this feature may be needed for other > arch/plat. So having a generic option could be useful for future > changes. > > Thomas, do you have an opinion ? > > Can we consider adding a set of kernel parameters instead of adding > DT exceptions time to time ? > > eg. > > clocksource.reset = <0/1> > clocksource.virtual = <0/1> I don't think you can have generic optins for this. What exactly you need will depend on the specific clocksource and set of other agents using it. Generic options won't adequately capture those details. Thanks, Mark.
diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt index bdae3a8..bb897a9 100644 --- a/Documentation/devicetree/bindings/arm/global_timer.txt +++ b/Documentation/devicetree/bindings/arm/global_timer.txt @@ -17,6 +17,12 @@ - clocks : Should be phandle to a clock. + +** Optional properties: + +- arm,gt_no_reset : Firmware/bootloader already initialized the + global timer-counter and expects it to be not reset again. + Example: timer@2c000600 { diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index a2cb6fa..952bab6 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -51,6 +51,7 @@ static void __iomem *gt_base; static unsigned long gt_clk_rate; static int gt_ppi; static struct clock_event_device __percpu *gt_evt; +static bool gt_reset_counter; /* * To get the value from the Global Timer Counter register proceed as follows: @@ -212,9 +213,11 @@ static u64 notrace gt_sched_clock_read(void) static void __init gt_clocksource_init(void) { - writel(0, gt_base + GT_CONTROL); - writel(0, gt_base + GT_COUNTER0); - writel(0, gt_base + GT_COUNTER1); + if (gt_reset_counter) { + writel(0, gt_base + GT_CONTROL); + writel(0, gt_base + GT_COUNTER0); + writel(0, gt_base + GT_COUNTER1); + } /* enables timer on all the cores */ writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); @@ -303,6 +306,9 @@ static void __init global_timer_of_register(struct device_node *np) goto out_irq; } + /* See if we are told we can't reset the global timer counter */ + gt_reset_counter = !of_property_read_bool(np, "arm,gt_no_reset"); + /* Immediately configure the timer on the boot CPU */ gt_clocksource_init(); gt_clockevents_init(this_cpu_ptr(gt_evt));
The GT counter is common to every core in a cluster. There is a usecase when Linux is spawned by a 'master' firmware/OS running on some core of the same cluster and the GT is used by the both. Linux, upon boot, resetting the GT counter is obviously fatal to the other OS. So provide a way for DT to tell Linux if it's running in that 'slave' mode and must not reset the counter. Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> --- Documentation/devicetree/bindings/arm/global_timer.txt | 6 ++++++ drivers/clocksource/arm_global_timer.c | 12 +++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-)