[RFC,0/3] Add HiSilicon system timer driver

  From: Yicong Yang <yangyicong@hisilicon.com>

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
  

Hi,

On Tue, Oct 10, 2023 at 08:30:30PM +0800, Yicong Yang wrote:
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> 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 <yangyicong@huawei.com> wrote:
> 
> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> 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.
  

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 <yangyicong@hisilicon.com>
>>
>> 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 <yangyicong@huawei.com> wrote:
>>
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> 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 <yangyicong@huawei.com> wrote:
> >>
> >> From: Yicong Yang <yangyicong@hisilicon.com>
> >>
> >> 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 <yangyicong@huawei.com> wrote:
>>>>
>>>> From: Yicong Yang <yangyicong@hisilicon.com>
>>>>
>>>> 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 <yangyicong@huawei.com> wrote:
>>>>>
>>>>> From: Yicong Yang <yangyicong@hisilicon.com>
>>>>>
>>>>> 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