From: Yicong Yang <[email protected]>
HiSilicon system timer is a memory mapped platform timer compatible with
the arm's generic timer specification. The timer supports both SPI and
LPI interrupt and can be enumerated through ACPI DSDT table. Since the
timer is fully compatible with the spec, it can reuse most codes of the
arm_arch_timer driver. However since the arm_arch_timer driver only
supports GTDT and SPI interrupt, this series support the HiSilicon system
timer by:
- refactor some of the arm_arch_timer codes and export the function to
register a arch memory timer by other drivers
- retrieve the IO memory and interrupt resource through DSDT in a separate
driver, then setup and register the clockevent device reuse the arm_arch_timer
function
Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
Yicong Yang (3):
clocksource/drivers/arm_arch_timer: Split the function of
__arch_timer_setup()
clocksource/drivers/arm_arch_timer: Extend and export
arch_timer_mem_register()
clocksource/drivers: Add HiSilicon system timer driver
drivers/clocksource/Kconfig | 10 +++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
drivers/clocksource/timer-hisi-sys.c | 68 +++++++++++++++
include/clocksource/arm_arch_timer.h | 2 +
5 files changed, 148 insertions(+), 56 deletions(-)
create mode 100644 drivers/clocksource/timer-hisi-sys.c
--
2.24.0
From: Yicong Yang <[email protected]>
Currently the memory-mapped timer will be probed by the GTDT table on
ACPI based systems, and the timer interrupt can only be the GSI (SPI
interrupt for arm64) per ACPI spec. However the generic timer
specification doesn't restrict the interrupt type and per BSA Spec
section 3.8.1 (DEN0094C 1.0C) the timer interrupt can also be a LPI
interrupt.
So this patch extends and exports the arch_timer_mem_register() function
to allow other drivers registers a generic timer using LPI interrupt
and probed by other means rather than GTDT. Note that the GTDT timer
still has a higher priority, if a GTDT timer is registered, we'll block
later registration of other timers.
Signed-off-by: Yicong Yang <[email protected]>
---
drivers/clocksource/arm_arch_timer.c | 26 +++++++++++++++++---------
include/clocksource/arm_arch_timer.h | 2 ++
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2e20a8ec50ca..6e9445192ca4 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -66,7 +66,7 @@ struct arch_timer {
struct clock_event_device evt;
};
-static struct arch_timer *arch_timer_mem __ro_after_init;
+static struct arch_timer *arch_timer_mem;
#define to_arch_timer(e) container_of(e, struct arch_timer, evt)
@@ -888,15 +888,16 @@ static void __arch_timer_setup_cp15(struct clock_event_device *clk)
clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
}
-static void __arch_timer_setup_mem(struct clock_event_device *clk)
+static void __arch_timer_setup_mem(struct clock_event_device *clk,
+ bool irq_virtual, const char *name)
{
u64 max_delta;
clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ;
- clk->name = "arch_mem_timer";
+ clk->name = name;
clk->rating = 400;
clk->cpumask = cpu_possible_mask;
- if (arch_timer_mem_use_virtual) {
+ if (irq_virtual) {
clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;
clk->set_next_event =
@@ -1286,25 +1287,30 @@ static int __init arch_timer_register(void)
return err;
}
-static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
+int arch_timer_mem_register(void __iomem *base, unsigned int irq,
+ bool irq_virtual, const char *name)
{
int ret;
irq_handler_t func;
+ /* If we've already register a memory timer, fail the registration */
+ if (arch_timer_mem)
+ return -EEXIST;
+
arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
if (!arch_timer_mem)
return -ENOMEM;
arch_timer_mem->base = base;
arch_timer_mem->evt.irq = irq;
- __arch_timer_setup_mem(&arch_timer_mem->evt);
+ __arch_timer_setup_mem(&arch_timer_mem->evt, irq_virtual, name);
- if (arch_timer_mem_use_virtual)
+ if (irq_virtual)
func = arch_timer_handler_virt_mem;
else
func = arch_timer_handler_phys_mem;
- ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
+ ret = request_irq(irq, func, IRQF_TIMER, name, &arch_timer_mem->evt);
if (ret) {
pr_err("Failed to request mem timer irq\n");
kfree(arch_timer_mem);
@@ -1313,6 +1319,7 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
return ret;
}
+EXPORT_SYMBOL_GPL(arch_timer_mem_register);
static const struct of_device_id arch_timer_of_match[] __initconst = {
{ .compatible = "arm,armv7-timer", },
@@ -1560,7 +1567,8 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
return -ENXIO;
}
- ret = arch_timer_mem_register(base, irq);
+ ret = arch_timer_mem_register(base, irq, arch_timer_mem_use_virtual,
+ "arch_mem_timer");
if (ret) {
iounmap(base);
return ret;
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index cbbc9a6dc571..d0fa2065586c 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -89,6 +89,8 @@ extern u32 arch_timer_get_rate(void);
extern u64 (*arch_timer_read_counter)(void);
extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
extern bool arch_timer_evtstrm_available(void);
+extern int arch_timer_mem_register(void __iomem *base, unsigned int irq,
+ bool irq_virtual, const char *name);
#else
--
2.24.0
Hi,
On Tue, Oct 10, 2023 at 08:30:30PM +0800, Yicong Yang wrote:
> From: Yicong Yang <[email protected]>
>
> HiSilicon system timer is a memory mapped platform timer compatible with
> the arm's generic timer specification. The timer supports both SPI and
> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
> timer is fully compatible with the spec, it can reuse most codes of the
> arm_arch_timer driver. However since the arm_arch_timer driver only
> supports GTDT and SPI interrupt, this series support the HiSilicon system
> timer by:
>
> - refactor some of the arm_arch_timer codes and export the function to
> register a arch memory timer by other drivers
> - retrieve the IO memory and interrupt resource through DSDT in a separate
> driver, then setup and register the clockevent device reuse the arm_arch_timer
> function
>
> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
This seems like an oversight; there *should* be a generic way of describing
this, and I've poked our BSA and ACPI architects to figure out how this is
supposed to work. The lack of a way to do that seems like a major oversight and
something that needs to be solved.
I'll try to get back to you shortly on that.
Regardless of that, I do not think this should be a separate driver, and I'm
very much not keen on having vendor-specific companion drivers like this. Using
LPIs isn't specific to HiSilicon, and this should be entirely common (and if we
need a DSDT device, should use a common HID too).
Thanks,
Mark.
>
> Yicong Yang (3):
> clocksource/drivers/arm_arch_timer: Split the function of
> __arch_timer_setup()
> clocksource/drivers/arm_arch_timer: Extend and export
> arch_timer_mem_register()
> clocksource/drivers: Add HiSilicon system timer driver
>
> drivers/clocksource/Kconfig | 10 +++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
> drivers/clocksource/timer-hisi-sys.c | 68 +++++++++++++++
> include/clocksource/arm_arch_timer.h | 2 +
> 5 files changed, 148 insertions(+), 56 deletions(-)
> create mode 100644 drivers/clocksource/timer-hisi-sys.c
>
> --
> 2.24.0
>
On Tue, 10 Oct 2023 13:30:30 +0100,
Yicong Yang <[email protected]> wrote:
>
> From: Yicong Yang <[email protected]>
>
> HiSilicon system timer is a memory mapped platform timer compatible with
> the arm's generic timer specification. The timer supports both SPI and
> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
> timer is fully compatible with the spec, it can reuse most codes of the
> arm_arch_timer driver. However since the arm_arch_timer driver only
> supports GTDT and SPI interrupt, this series support the HiSilicon system
> timer by:
>
> - refactor some of the arm_arch_timer codes and export the function to
> register a arch memory timer by other drivers
> - retrieve the IO memory and interrupt resource through DSDT in a separate
> driver, then setup and register the clockevent device reuse the arm_arch_timer
> function
>
> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
This strikes me as pretty odd. LPIs are, by definition, *edge*
triggered. The timer interrupt must be *level* triggered. So there
must be some bridge in the middle that is going to regenerate edges on
EOI, and that cannot be architectural.
What am I missing?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On 2023/10/10 23:43, Mark Rutland wrote:
> Hi,
>
> On Tue, Oct 10, 2023 at 08:30:30PM +0800, Yicong Yang wrote:
>> From: Yicong Yang <[email protected]>
>>
>> HiSilicon system timer is a memory mapped platform timer compatible with
>> the arm's generic timer specification. The timer supports both SPI and
>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>> timer is fully compatible with the spec, it can reuse most codes of the
>> arm_arch_timer driver. However since the arm_arch_timer driver only
>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>> timer by:
>>
>> - refactor some of the arm_arch_timer codes and export the function to
>> register a arch memory timer by other drivers
>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>> driver, then setup and register the clockevent device reuse the arm_arch_timer
>> function
>>
>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
>
> This seems like an oversight; there *should* be a generic way of describing
> this, and I've poked our BSA and ACPI architects to figure out how this is
> supposed to work. The lack of a way to do that seems like a major oversight and
> something that needs to be solved.
>
> I'll try to get back to you shortly on that.
>
Looking forward to the conclusion/solution.
> Regardless of that, I do not think this should be a separate driver, and I'm
> very much not keen on having vendor-specific companion drivers like this. Using
> LPIs isn't specific to HiSilicon, and this should be entirely common (and if we
> need a DSDT device, should use a common HID too).
>
This is reasonable. Actually we're using most funtions of the arch timer driver, only
do the resource enumeration in the separate driver which is lack in the arch timer driver.
So if there's a common solution in the arch timer driver as well as a common HID we're
willing to use it.
Thanks.
> Thanks,
> Mark.
>
>>
>> Yicong Yang (3):
>> clocksource/drivers/arm_arch_timer: Split the function of
>> __arch_timer_setup()
>> clocksource/drivers/arm_arch_timer: Extend and export
>> arch_timer_mem_register()
>> clocksource/drivers: Add HiSilicon system timer driver
>>
>> drivers/clocksource/Kconfig | 10 +++
>> drivers/clocksource/Makefile | 1 +
>> drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
>> drivers/clocksource/timer-hisi-sys.c | 68 +++++++++++++++
>> include/clocksource/arm_arch_timer.h | 2 +
>> 5 files changed, 148 insertions(+), 56 deletions(-)
>> create mode 100644 drivers/clocksource/timer-hisi-sys.c
>>
>> --
>> 2.24.0
>>
>
> .
>
On 2023/10/11 0:36, Marc Zyngier wrote:
> On Tue, 10 Oct 2023 13:30:30 +0100,
> Yicong Yang <[email protected]> wrote:
>>
>> From: Yicong Yang <[email protected]>
>>
>> HiSilicon system timer is a memory mapped platform timer compatible with
>> the arm's generic timer specification. The timer supports both SPI and
>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>> timer is fully compatible with the spec, it can reuse most codes of the
>> arm_arch_timer driver. However since the arm_arch_timer driver only
>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>> timer by:
>>
>> - refactor some of the arm_arch_timer codes and export the function to
>> register a arch memory timer by other drivers
>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>> driver, then setup and register the clockevent device reuse the arm_arch_timer
>> function
>>
>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
>
> This strikes me as pretty odd. LPIs are, by definition, *edge*
> triggered. The timer interrupt must be *level* triggered. So there
> must be some bridge in the middle that is going to regenerate edges on
> EOI, and that cannot be architectural.
>
> What am I missing?
In our case, if the timer is working on LPI mode, it's not directly connected
to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
GIC.
Thanks.
On Wed, Oct 11, 2023 at 10:10:11AM +0800, Yicong Yang wrote:
> On 2023/10/11 0:36, Marc Zyngier wrote:
> > On Tue, 10 Oct 2023 13:30:30 +0100,
> > Yicong Yang <[email protected]> wrote:
> >>
> >> From: Yicong Yang <[email protected]>
> >>
> >> HiSilicon system timer is a memory mapped platform timer compatible with
> >> the arm's generic timer specification. The timer supports both SPI and
> >> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
> >> timer is fully compatible with the spec, it can reuse most codes of the
> >> arm_arch_timer driver. However since the arm_arch_timer driver only
> >> supports GTDT and SPI interrupt, this series support the HiSilicon system
> >> timer by:
> >>
> >> - refactor some of the arm_arch_timer codes and export the function to
> >> register a arch memory timer by other drivers
> >> - retrieve the IO memory and interrupt resource through DSDT in a separate
> >> driver, then setup and register the clockevent device reuse the arm_arch_timer
> >> function
> >>
> >> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
> >
> > This strikes me as pretty odd. LPIs are, by definition, *edge*
> > triggered. The timer interrupt must be *level* triggered. So there
> > must be some bridge in the middle that is going to regenerate edges on
> > EOI, and that cannot be architectural.
> >
> > What am I missing?
>
> In our case, if the timer is working on LPI mode, it's not directly connected
> to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
> GIC.
In that case, the timerr itself isn't using an LPI: it's wired to a secondary
interrupt controller, and the secondary interrupt controller is using an LPI.
The BSA doesn't describe that as a permitted configuration.
I think there are two problems here:
(1) The BSA spec is wrong, and shouldn't say "or LPI" here as it simply doesn't
make sense.
I think this should be fixed by removing the "or LPI" wording form the BSA
spec for this interrupt.
(2) This platform is not compatible with the BSA, and is not compatible with
the existing ACPI bindings in the GTDT.
Do you actually need this wakeup timer?
Thanks,
Mark.
Hi Mark,
On 2023/10/11 18:38, Mark Rutland wrote:
> On Wed, Oct 11, 2023 at 10:10:11AM +0800, Yicong Yang wrote:
>> On 2023/10/11 0:36, Marc Zyngier wrote:
>>> On Tue, 10 Oct 2023 13:30:30 +0100,
>>> Yicong Yang <[email protected]> wrote:
>>>>
>>>> From: Yicong Yang <[email protected]>
>>>>
>>>> HiSilicon system timer is a memory mapped platform timer compatible with
>>>> the arm's generic timer specification. The timer supports both SPI and
>>>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>>>> timer is fully compatible with the spec, it can reuse most codes of the
>>>> arm_arch_timer driver. However since the arm_arch_timer driver only
>>>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>>>> timer by:
>>>>
>>>> - refactor some of the arm_arch_timer codes and export the function to
>>>> register a arch memory timer by other drivers
>>>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>>> driver, then setup and register the clockevent device reuse the arm_arch_timer
>>>> function
>>>>
>>>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
>>>
>>> This strikes me as pretty odd. LPIs are, by definition, *edge*
>>> triggered. The timer interrupt must be *level* triggered. So there
>>> must be some bridge in the middle that is going to regenerate edges on
>>> EOI, and that cannot be architectural.
>>>
>>> What am I missing?
>>
>> In our case, if the timer is working on LPI mode, it's not directly connected
>> to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
>> GIC.
>
> In that case, the timerr itself isn't using an LPI: it's wired to a secondary
> interrupt controller, and the secondary interrupt controller is using an LPI.
>
> The BSA doesn't describe that as a permitted configuration.
>
> I think there are two problems here:
>
> (1) The BSA spec is wrong, and shouldn't say "or LPI" here as it simply doesn't
> make sense.
>
> I think this should be fixed by removing the "or LPI" wording form the BSA
> spec for this interrupt.
>
> (2) This platform is not compatible with the BSA, and is not compatible with
> the existing ACPI bindings in the GTDT.
>
> Do you actually need this wakeup timer?
>
Thanks for the quick feedback!
So the LPI timer mentioned in the BSA spec is probably a mistake and our LPI
mode is not compatible to the BSA spec. Then I need to discuss with my team
and re-evaluate the solution for the LPI mode of the timer.
Thanks,
Yicong
Hi Mark,
On 2023/10/11 21:10, Yicong Yang wrote:
> Hi Mark,
>
> On 2023/10/11 18:38, Mark Rutland wrote:
>> On Wed, Oct 11, 2023 at 10:10:11AM +0800, Yicong Yang wrote:
>>> On 2023/10/11 0:36, Marc Zyngier wrote:
>>>> On Tue, 10 Oct 2023 13:30:30 +0100,
>>>> Yicong Yang <[email protected]> wrote:
>>>>>
>>>>> From: Yicong Yang <[email protected]>
>>>>>
>>>>> HiSilicon system timer is a memory mapped platform timer compatible with
>>>>> the arm's generic timer specification. The timer supports both SPI and
>>>>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>>>>> timer is fully compatible with the spec, it can reuse most codes of the
>>>>> arm_arch_timer driver. However since the arm_arch_timer driver only
>>>>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>>>>> timer by:
>>>>>
>>>>> - refactor some of the arm_arch_timer codes and export the function to
>>>>> register a arch memory timer by other drivers
>>>>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>>>> driver, then setup and register the clockevent device reuse the arm_arch_timer
>>>>> function
>>>>>
>>>>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
>>>>
>>>> This strikes me as pretty odd. LPIs are, by definition, *edge*
>>>> triggered. The timer interrupt must be *level* triggered. So there
>>>> must be some bridge in the middle that is going to regenerate edges on
>>>> EOI, and that cannot be architectural.
>>>>
>>>> What am I missing?
>>>
>>> In our case, if the timer is working on LPI mode, it's not directly connected
>>> to the GIC. It'll be wired to hisi-mbigen irqchip which will send LPIs to the
>>> GIC.
>>
>> In that case, the timerr itself isn't using an LPI: it's wired to a secondary
>> interrupt controller, and the secondary interrupt controller is using an LPI.
>>
>> The BSA doesn't describe that as a permitted configuration.
>>
>> I think there are two problems here:
>>
>> (1) The BSA spec is wrong, and shouldn't say "or LPI" here as it simply doesn't
>> make sense.
>>
>> I think this should be fixed by removing the "or LPI" wording form the BSA
>> spec for this interrupt.
>>
>> (2) This platform is not compatible with the BSA, and is not compatible with
>> the existing ACPI bindings in the GTDT.
>>
>> Do you actually need this wakeup timer?
>>
>
> Thanks for the quick feedback!
>
> So the LPI timer mentioned in the BSA spec is probably a mistake and our LPI
> mode is not compatible to the BSA spec. Then I need to discuss with my team
> and re-evaluate the solution for the LPI mode of the timer.
>
Since our system timer interrupt supports both SPI and non-SPI mode. For some cases
we want to leave SPI interrupt for secure system and the timer for non-secure
system (linux) will be wired to the hisi-mbigen. Just want to confirm if the
solution to support the non-SPI mode in this patchset is acceptable for our use case,
though it's not permitted by BSA spec?
Thanks,
Yicong