Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Mark Rutland]

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@xxxxxxxxxx> wrote:
> >>
> >> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
> >>
> >> 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.