| Message ID | 20170118132541.8989-1-fu.wei@linaro.org |
|---|---|
| Headers | show |
| Series | acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer | expand |
Hi Fuwei, One comments below. On 2017/1/18 21:25, fu.wei@linaro.org wrote: > From: Fu Wei <fu.wei@linaro.org> > > The counter frequency detection call(arch_timer_detect_rate) combines two > ways to get counter frequency: system coprocessor register and MMIO timer. > But in a specific timer init code, we only need one way to try: > getting frequency from MMIO timer register will be needed only when we > init MMIO timer; getting frequency from system coprocessor register will > be needed only when we init arch timer. > > This patch separates paths to determine frequency: > Separate out the MMIO frequency and the sysreg frequency detection call, > and use the appropriate one for the counter. > > Signed-off-by: Fu Wei <fu.wei@linaro.org> > --- > drivers/clocksource/arm_arch_timer.c | 40 ++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c > index 6484f84..9482481 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -488,23 +488,33 @@ static int arch_timer_starting_cpu(unsigned int cpu) > return 0; > } > > -static void arch_timer_detect_rate(void __iomem *cntbase) > +static void __arch_timer_determine_rate(u32 rate) > { > - /* Who has more than one independent system counter? */ > - if (arch_timer_rate) > - return; > + /* Check the timer frequency. */ > + if (!arch_timer_rate) { > + if (rate) > + arch_timer_rate = rate; > + else > + pr_warn("frequency not available\n"); > + } else if (rate && arch_timer_rate != rate) { ^ Typo? I think it's "&" here. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hanjun, On 19 January 2017 at 17:16, Hanjun Guo <hanjun.guo@linaro.org> wrote: > On 2017/1/18 21:25, fu.wei@linaro.org wrote: >> >> From: Fu Wei <fu.wei@linaro.org> >> >> The patch add memory-mapped timer register support by using the >> information provided by the new GTDT driver of ACPI. >> >> Signed-off-by: Fu Wei <fu.wei@linaro.org> >> --- >> drivers/clocksource/arm_arch_timer.c | 35 >> ++++++++++++++++++++++++++++++++--- >> 1 file changed, 32 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clocksource/arm_arch_timer.c >> b/drivers/clocksource/arm_arch_timer.c >> index 79dc004..7ca2da7 100644 >> --- a/drivers/clocksource/arm_arch_timer.c >> +++ b/drivers/clocksource/arm_arch_timer.c >> @@ -1077,10 +1077,36 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, >> "arm,armv7-timer-mem", >> arch_timer_mem_of_init); >> >> #ifdef CONFIG_ACPI_GTDT >> -/* Initialize per-processor generic timer */ >> +static int __init arch_timer_mem_acpi_init(int platform_timer_count) >> +{ >> + struct arch_timer_mem *timer_mem; >> + int timer_count, i, ret; >> + > > > if (!platform_timer_count) > return 0; > > Did I miss something? Ah, thanks, I guess I miss this check below. > > Thanks > Hanjun > > >> + timer_mem = kcalloc(platform_timer_count, sizeof(*timer_mem), >> + GFP_KERNEL); >> + if (!timer_mem) >> + return -ENOMEM; >> + >> + ret = acpi_arch_timer_mem_init(timer_mem, &timer_count); >> + if (ret || !timer_count) >> + goto error; >> + >> + for (i = 0; i < timer_count; i++) { >> + ret = arch_timer_mem_init(timer_mem); >> + if (!ret) >> + break; >> + timer_mem++; >> + } >> + >> +error: >> + kfree(timer_mem); >> + return ret; >> +} >> + >> +/* Initialize per-processor generic timer and memory-mapped timer(if >> present) */ >> static int __init arch_timer_acpi_init(struct acpi_table_header *table) >> { >> - int ret; >> + int ret, platform_timer_count; >> >> if (arch_timers_present & ARCH_TIMER_TYPE_CP15) { >> pr_warn("already initialized, skipping\n"); >> @@ -1089,7 +1115,7 @@ static int __init arch_timer_acpi_init(struct >> acpi_table_header *table) >> >> arch_timers_present |= ARCH_TIMER_TYPE_CP15; >> >> - ret = acpi_gtdt_init(table, NULL); >> + ret = acpi_gtdt_init(table, &platform_timer_count); >> if (ret) { >> pr_err("Failed to init GTDT table.\n"); >> return ret; >> @@ -1122,6 +1148,9 @@ static int __init arch_timer_acpi_init(struct >> acpi_table_header *table) >> if (ret) >> return ret; >> >> + if (arch_timer_mem_acpi_init(platform_timer_count)) + if (platform_timer_count && + arch_timer_mem_acpi_init(platform_timer_count)) >> + pr_err("Failed to initialize memory-mapped timer.\n"); >> + >> return arch_timer_common_init(); >> } >> CLOCKSOURCE_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, >> arch_timer_acpi_init); >> > -- Best regards, Fu Wei Software Engineer Red Hat -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Fu, On 01/25/2017 01:46 AM, Fu Wei wrote: > Hi Mark, > > On 25 January 2017 at 01:24, Mark Rutland <mark.rutland@arm.com> wrote: >> On Wed, Jan 18, 2017 at 09:25:32PM +0800, fu.wei@linaro.org wrote: >>> From: Fu Wei <fu.wei@linaro.org> >>> >>> The counter frequency detection call(arch_timer_detect_rate) combines two >>> ways to get counter frequency: system coprocessor register and MMIO timer. >>> But in a specific timer init code, we only need one way to try: >>> getting frequency from MMIO timer register will be needed only when we >>> init MMIO timer; getting frequency from system coprocessor register will >>> be needed only when we init arch timer. >> >> When I mentioned this splitting before, I had mean that we'd completely >> separate the two, with separate mmio_rate and sysreg_rate variables. > > sorry for misunderstanding. > > Are you saying : > > diff --git a/drivers/clocksource/arm_arch_timer.c > b/drivers/clocksource/arm_arch_timer.c > index 663a57a..eec92f6 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -65,7 +65,8 @@ struct arch_timer { > > #define to_arch_timer(e) container_of(e, struct arch_timer, evt) > > -static u32 arch_timer_rate; > +static u32 arch_timer_sysreg_rate ; > +static u32 arch_timer_mmio_rate; > static int arch_timer_ppi[ARCH_TIMER_MAX_TIMER_PPI]; > > static struct clock_event_device __percpu *arch_timer_evt; > > > But what have I learned From ARMv8 ARM is > AArch64 System register CNTFRQ_EL0 is provided so that software can > discover the frequency of the system counter. > CNTFRQ(in CNTCTLBase and CNTBaseN) is provided so that software can > discover the frequency of the system counter. > The bit assignments of the registers are identical in the System > register interface and in the memory-mapped system level interface. > So I think they both contain the same value : the frequency of the > system counter, just in different view, and can be accessed in > different ways. > > So do we really need to separate mmio_rate and sysreg_rate variables? > > And for CNTFRQ(in CNTCTLBase and CNTBaseN) , we can NOT access it in > Linux kernel (EL1), > Because ARMv8 ARM says: > In a system that implements both Secure and Non-secure states, this > register is only accessible by Secure accesses. > That means we still need to get the frequency of the system counter > from CNTFRQ_EL0 in MMIO timer code. > This have been proved when I tested this driver on foundation model, I > got "0" when I access CNTFRQ from Linux kernel (Non-secure EL1) That sounds like a firmware problem. Firmware in EL3 is supposed to write the value into CNTFRQ. If you're not currently using any firmware, I'd recommend the bootwrapper on models/simulators/emulators. http://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/tree/arch/aarch64/boot.S#n48 Cheers, Cov -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 25, 2017 at 10:38:01AM -0500, Christopher Covington wrote: > On 01/25/2017 01:46 AM, Fu Wei wrote: > > On 25 January 2017 at 01:24, Mark Rutland <mark.rutland@arm.com> wrote: > >> On Wed, Jan 18, 2017 at 09:25:32PM +0800, fu.wei@linaro.org wrote: > >>> From: Fu Wei <fu.wei@linaro.org> > > And for CNTFRQ(in CNTCTLBase and CNTBaseN) , we can NOT access it in > > Linux kernel (EL1), > > Because ARMv8 ARM says: > > In a system that implements both Secure and Non-secure states, this > > register is only accessible by Secure accesses. > > That means we still need to get the frequency of the system counter > > from CNTFRQ_EL0 in MMIO timer code. > > This have been proved when I tested this driver on foundation model, I > > got "0" when I access CNTFRQ from Linux kernel (Non-secure EL1) > > That sounds like a firmware problem. Firmware in EL3 is supposed to write > the value into CNTFRQ. Definitely. FW *should* program the CNTFRQ_EL0 CPU registers and any MMIO CNTFRQ registers. > If you're not currently using any firmware, I'd > recommend the bootwrapper on models/simulators/emulators. > > http://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/tree/arch/aarch64/boot.S#n48 Unfortunately, the boot-wrapper only programs the CNTFRQ_EL0 CPU system registers, and does not program any MMIO CNTFRQ registers. IIRC the models it was originally written for didn't have any (and we had no DT binding until far later...). Luckily the model DTs do not expose any MMIO timer addresses to the kernel currently. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 01, 2017 at 02:43:02AM +0800, Fu Wei wrote: > On 31 January 2017 at 01:49, Mark Rutland <mark.rutland@arm.com> wrote: > > On Thu, Jan 26, 2017 at 01:49:03PM +0800, Fu Wei wrote: > >> On 26 January 2017 at 01:25, Mark Rutland <mark.rutland@arm.com> wrote: > >> > On Wed, Jan 25, 2017 at 02:46:12PM +0800, Fu Wei wrote: > >> >> On 25 January 2017 at 01:24, Mark Rutland <mark.rutland@arm.com> wrote: > >> >> > On Wed, Jan 18, 2017 at 09:25:32PM +0800, fu.wei@linaro.org wrote: > >> >> >> From: Fu Wei <fu.wei@linaro.org> > > > >> But according to another document(ARMv8-A Foundation Platform User > >> Guide ARM DUI0677K), > >> Table 3-2 ARMv8-A Foundation Platform memory map (continued) > >> > >> AP_REFCLK CNTBase0, Generic Timer 64KB S > >> AP_REFCLK CNTBase1, Generic Timer 64KB S/NS > >> > >> Dose it means the timer frame 0 can be accessed in SECURE status only, > >> and the timer frame 1 can be accessed in both status? > > > > That does appear to be what it says. > > > > I assume in this case CNTCTLBase.CNTSAR<0> is RES0. > > > >> And because Linux kernel is running on Non-secure EL1, so should we > >> skip "SECURE" timer in Linux? > > > > I guess you mean by checking the GTx Common flags, to see if the timer > > is secure? Yes, we must skip those. > > Yes, exactly. > > I think we can check the GTx Common flags, if the timer is set as > SECURE, this driver should just skip this timer. I completely agree that we must skip these. > > Looking further at this, the ACPI spec is sorely lacking any statement > > as to the configuration of CNTCTLBase.{CNTSAR,CNTTIDR,CNTACR}, so it's > > not clear if we can access anything in a frame, even if it is listed as > > being a non-secure timer. > > > > I think we need a stronger statement here. Otherwise, we will encounter > > problems. Linux currently assumes that CNTCTLBase.CNTACR<N> is > > writeable, given a non-secure frame N. This is only the case if > > CNTCTLBase.CNTSAR.NS<N> == 1. > > the original driver has checked these registers, but the problem is: > What if the timer frame is designed to be a secure timer, all the > register in this frame is only can be accessed in secure status, just > like foundation model? > Note: for foundation model, Please check Table 3-1 Access permissions > of 3.1 ARMv8-A Foundation Platform memory map in ARMv8-A Foundation > Platform User Guide > > So I think we should check the GTDT first, if it's not a secure timer, > then we can go on checking CNTSAR. :-) I've clearly confused matters here. I completely agree that we must skip timers the GTDT descrbies as secure. My complaint here is that the spec does not explicitly state that CNTCTLBase.CNTSAR.NS<N> must be set for timers *not* marked as secure (though I believe that is the intent). That is a spec issue, not a code issue. We unfortunately can't check CNTNSAR, as it is secure-only. :( Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Fu Wei <fu.wei@linaro.org> This patchset: (1)Preparation for adding GTDT support in arm_arch_timer: 1. Clean up printk() usage 2. Rename the type macros 3. Rename the PPI enum & enum values 4. Move the type macro and PPI enum into the header file 5. Add new enum for SPIs 6. Rework PPI determination; 7. Rework counter frequency detection; 8. Refactor arch_timer_needs_probing, move it into DT init call 9. Introduce some new structs and refactor the MMIO timer init code for reusing some common code. (2)Introduce ACPI GTDT parser: drivers/acpi/arm64/acpi_gtdt.c Parse all kinds of timer in GTDT table of ACPI:arch timer, memory-mapped timer and SBSA Generic Watchdog timer. This driver can help to simplify all the relevant timer drivers, and separate all the ACPI GTDT knowledge from them. (3)Simplify ACPI code for arm_arch_timer (4)Add GTDT support for ARM memory-mapped timer. This patchset has been tested on the following platforms with ACPI enabled: (1)ARM Foundation v8 model Changelog: v20: https://lkml.org/lkml/2017/1/18/ Reorder the first 4 patches and split the 4th patches. Leave CNTHCTL_* as they originally were. Fix the bug in arch_timer_select_ppi. Split "Rework counter frequency detection" patch. Rework the arch_timer_detect_rate function. Improve the commit message of "Refactor MMIO timer probing". Rebase to 4.10.0-rc4 v19: https://lkml.org/lkml/2016/12/21/25 Fix a '\n' missing in a error message in arch_timer_mem_init. Add "request_mem_region" for ioremapping cntbase, according to f947ee1 clocksource/drivers/arm_arch_timer: Map frame with of_io_request_and_map() Rebase to 4.9.0-gfb779ff v18: https://lkml.org/lkml/2016/12/8/446 Fix 8/15 patch problem of "int ret;" in arch_timer_acpi_init. Rebase to 4.9.0-rc8-g9269898 v17: https://lkml.org/lkml/2016/11/25/140 Take out some cleanups from 4/15. Merge 5/15 and 6/15, improve PPI determination code, improve commit message. Rework counter frequency detection. Move arch_timer_needs_of_probing into DT init call. Move Platform Timer scan loop back to timer init call to avoid allocating and free memory. Improve all the exported functions' comment. v16: https://lkml.org/lkml/2016/11/16/268 Fix patchset problem about static enum ppi_nr of 01/13 in v15. Refactor arch_timer_detect_rate. Refactor arch_timer_needs_probing. v15: https://lkml.org/lkml/2016/11/15/366 Re-order patches Add arm_arch_timer refactoring patches to prepare for GTDT: 1. rename some enums and defines, and some cleanups 2. separate out arch_timer_uses_ppi init code and fix a potential bug 3. Improve some new structs, refactor the timer init code. Since the some structs have been changed, GTDT parser for memory-mapped timer and SBSA Generic Watchdog timer have been update. v14: https://lkml.org/lkml/2016/9/28/573 Separate memory-mapped timer GTDT support into two patches 1. Refactor the timer init code to prepare for GTDT 2. Add GTDT support for memory-mapped timer v13: http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1231717.html Improve arm_arch_timer code for memory-mapped timer GTDT support, refactor original memory-mapped timer dt support for reusing some common code. v12: https://lkml.org/lkml/2016/9/13/250 Rebase to latest Linux 4.8-rc6 Delete the confusing "skipping" in the error message. V11: https://lkml.org/lkml/2016/9/6/354 Rebase to latest Linux 4.8-rc5 Delete typedef (suggested by checkpatch.pl) V10: https://lkml.org/lkml/2016/7/26/215 Drop the "readq" patch. Rebase to latest Linux 4.7. V9: https://lkml.org/lkml/2016/7/25/345 Improve pr_err message in acpi gtdt driver. Update Commit message for 7/9 shorten the irq mapping function name Improve GTDT driver for memory-mapped timer v8: https://lkml.org/lkml/2016/7/19/660 Improve "pr_fmt(fmt)" definition: add "ACPI" in front of "GTDT", and also improve printk message. Simplify is_timer_block and is_watchdog. Merge acpi_gtdt_desc_init and gtdt_arch_timer_init into acpi_gtdt_init(); Delete __init in include/linux/acpi.h for GTDT API Make ARM64 select GTDT. Delete "#include <linux/module.h>" from acpi_gtdt.c Simplify GT block parse code. v7: https://lkml.org/lkml/2016/7/13/769 Move the GTDT driver to drivers/acpi/arm64 Add add the ARM64-specific ACPI Support maintainers in MAINTAINERS Merge 3 patches of GTDT parser driver. Fix the for_each_platform_timer bug. v6: https://lkml.org/lkml/2016/6/29/580 split the GTDT driver to 4 parts: basic, arch_timer, memory-mapped timer, and SBSA Generic Watchdog timer Improve driver by suggestions and example code from Daniel Lezcano v5: https://lkml.org/lkml/2016/5/24/356 Sorting out all patches, simplify the API of GTDT driver: GTDT driver just fills the data struct for arm_arch_timer driver. v4: https://lists.linaro.org/pipermail/linaro-acpi/2016-March/006667.html Delete the kvm relevant patches Separate two patches for sorting out the code for arm_arch_timer. Improve irq info export code to allow missing irq info in GTDT table. v3: https://lkml.org/lkml/2016/2/1/658 Improve GTDT driver code: (1)improve pr_* by defining pr_fmt(fmt) (2)simplify gtdt_sbsa_gwdt_init (3)improve gtdt_arch_timer_data_init, if table is NULL, it will try to get GTDT table. Move enum ppi_nr to arm_arch_timer.h, and add enum spi_nr. Add arm_arch_timer get ppi from DT and GTDT support for kvm. v2: https://lkml.org/lkml/2015/12/2/10 Rebase to latest kernel version(4.4-rc3). Fix the bug about the config problem, use CONFIG_ACPI_GTDT instead of CONFIG_ACPI in arm_arch_timer.c v1: The first upstreaming version: https://lkml.org/lkml/2015/10/28/553 Fu Wei (17): clocksource/drivers/arm_arch_timer: Improve printk relevant code clocksource/drivers/arm_arch_timer: Rename the timer type macros. clocksource/drivers/arm_arch_timer: Rename the PPI enum and its values. clocksource/drivers/arm_arch_timer: Move enums and defines to header file. clocksource/drivers/arm_arch_timer: Add a new enum for spi type clocksource/drivers/arm_arch_timer: rework PPI determination clocksource/drivers/arm_arch_timer: Separate out device-tree code from arch_timer_detect_rate clocksource/drivers/arm_arch_timer: Rework counter frequency detection. clocksource/drivers/arm_arch_timer: Refactor arch_timer_needs_probing clocksource/drivers/arm_arch_timer: Move arch_timer_needs_of_probing into DT init call clocksource/drivers/arm_arch_timer: Introduce some new structs to prepare for GTDT clocksource/drivers/arm_arch_timer: Refactor MMIO timer probing. acpi/arm64: Add GTDT table parse driver clocksource/drivers/arm_arch_timer: Simplify ACPI support code. acpi/arm64: Add memory-mapped timer support in GTDT driver clocksource/drivers/arm_arch_timer: Add GTDT support for memory-mapped timer acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver arch/arm64/Kconfig | 1 + drivers/acpi/arm64/Kconfig | 3 + drivers/acpi/arm64/Makefile | 1 + drivers/acpi/arm64/gtdt.c | 374 ++++++++++++++++++++++++++ drivers/clocksource/arm_arch_timer.c | 497 +++++++++++++++++++++-------------- drivers/watchdog/Kconfig | 1 + include/clocksource/arm_arch_timer.h | 35 +++ include/linux/acpi.h | 7 + 8 files changed, 717 insertions(+), 202 deletions(-) create mode 100644 drivers/acpi/arm64/gtdt.c -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html