Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1525683

Re: [PATCH v16 06/15] clocksource/drivers/arm_arch_timer: separate out arch_timer_uses_ppi init code to prepare for GTDT.

Show all headers | View raw

On Wed, Nov 16, 2016 at 09:48:59PM +0800, fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
> 
> The patch refactor original arch_timer_uses_ppi init code:
> (1) Extract a subfunction: arch_timer_uses_ppi_init
> (2) Use the new subfunction in arch_timer_of_init and
> arch_timer_acpi_init

This isn't a strict refactoring, since this now assigns
ARCH_TIMER_PHYS_NONSECURE_PPI to arch_timer_uses_ppi, which we didn't do
previously.

As a general note, please write your commit messages as prose rather
than a list of bullet points. Please also explain the rationale for the
change, rather than enumerating the changes. Call out things which are
important and/or likely to surprise reviewers, for example:

* Can 32-bit ARM still use non-secure interrupts afer this change?

* Does the "arm,cpu-registers-not-fw-configured"  proeprty still work?

That will make it vastly easier to have this code reviewed, and it will
be far more helpful for anyone looking at this in future.

For example:

  arm_arch_timer: rework PPI determination

  Currently, the arch timer driver uses ARCH_TIMER_PHYS_SECURE_PPI to
  mean the driver will use the secure PPI *and* potentialy also use the
  non-secure PPI. This is somewhat confusing.

  For arm64, where it never makes sense to use the secure PPI, this
  means we must always request the useless secure PPI, adding to the
  confusion. For ACPI, where we may not even have a valid secure PPI
  number, this is additionally problematic. We need the driver to be
  able to use *only* the non-secure PPI.

  The logic to choose which PPI to use is intertwined with other logic
  in arch_timer_init(). This patch factors the PPI determination out
  into a new function, and then reworks it so that we can handle having
  only a non-secure PPI.

[...]

> +/*
> + * If HYP mode is available, we know that the physical timer
> + * has been configured to be accessible from PL1. Use it, so
> + * that a guest can use the virtual timer instead.
> + *
> + * If no interrupt provided for virtual timer, we'll have to
> + * stick to the physical timer. It'd better be accessible...
> + * On ARM64, we we only use ARCH_TIMER_PHYS_NONSECURE_PPI in Linux.

It would be better to say that for arm64 we never use the secure
interrupt.

> + *
> + * On ARMv8.1 with VH extensions, the kernel runs in HYP. VHE
> + * accesses to CNTP_*_EL1 registers are silently redirected to
> + * their CNTHP_*_EL2 counterparts, and use a different PPI
> + * number.
> + */
> +static int __init arch_timer_uses_ppi_init(void)

It would be better to call this something like arch_timer_select_ppi().
As it stands, the name is difficult to read.

> @@ -902,6 +904,10 @@ static int __init arch_timer_of_init(struct device_node *np)
>  	    of_property_read_bool(np, "arm,cpu-registers-not-fw-configured"))
>  		arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI;
>  
> +	ret = arch_timer_uses_ppi_init();
> +	if (ret)
> +		return ret;

This is clearly broken if you consider what the statement above is
doing.

Thanks,
Mark.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v16 00/15] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer fu.wei@linaro.org - 2016-11-16 14:50 +0100
  [PATCH v16 07/15] clocksource/drivers/arm_arch_timer: Refactor arch_timer_detect_rate to keep dt code in *_of_init fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 07/15] clocksource/drivers/arm_arch_timer: Refactor  arch_timer_detect_rate to keep dt code in *_of_init Mark Rutland <mark.rutland@arm.com> - 2016-11-18 21:00 +0100
      Re: [PATCH v16 07/15] clocksource/drivers/arm_arch_timer: Refactor  arch_timer_detect_rate to keep dt code in *_of_init Fu Wei <fu.wei@linaro.org> - 2016-11-21 15:10 +0100
  [PATCH v16 12/15] clocksource/drivers/arm_arch_timer: Simplify ACPI support code. fu.wei@linaro.org - 2016-11-16 15:00 +0100
  [PATCH v16 13/15] acpi/arm64: Add memory-mapped timer support in GTDT driver fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 13/15] acpi/arm64: Add memory-mapped timer support in  GTDT driver Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-18 15:30 +0100
      Re: [PATCH v16 13/15] acpi/arm64: Add memory-mapped timer support in  GTDT driver Fu Wei <fu.wei@linaro.org> - 2016-11-23 13:00 +0100
        Re: [PATCH v16 13/15] acpi/arm64: Add memory-mapped timer support in  GTDT driver Fu Wei <fu.wei@linaro.org> - 2016-11-24 05:00 +0100
  [PATCH v16 10/15] clocksource/drivers/arm_arch_timer: Refactor the timer init code to prepare for GTDT fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 10/15] clocksource/drivers/arm_arch_timer: Refactor  the timer init code to prepare for GTDT Mark Rutland <mark.rutland@arm.com> - 2016-11-18 21:10 +0100
      Re: [PATCH v16 10/15] clocksource/drivers/arm_arch_timer: Refactor  the timer init code to prepare for GTDT Fu Wei <fu.wei@linaro.org> - 2016-11-23 07:20 +0100
  [PATCH v16 11/15] acpi/arm64: Add GTDT table parse driver fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 11/15] acpi/arm64: Add GTDT table parse driver Mark Rutland <mark.rutland@arm.com> - 2016-11-18 21:20 +0100
      Re: [PATCH v16 11/15] acpi/arm64: Add GTDT table parse driver Fu Wei <fu.wei@linaro.org> - 2016-11-23 13:10 +0100
  [PATCH v16 04/15] clocksource/drivers/arm_arch_timer: rename some enums and defines, and some cleanups. fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 04/15] clocksource/drivers/arm_arch_timer: rename  some enums and defines, and some cleanups. Mark Rutland <mark.rutland@arm.com> - 2016-11-18 20:00 +0100
      Re: [PATCH v16 04/15] clocksource/drivers/arm_arch_timer: rename some  enums and defines, and some cleanups. Fu Wei <fu.wei@linaro.org> - 2016-11-21 07:20 +0100
  [PATCH v16 01/15] clocksource/drivers/arm_arch_timer: Move enums and defines to header file fu.wei@linaro.org - 2016-11-16 15:00 +0100
  [PATCH v16 15/15] acpi/arm64: Add SBSA Generic Watchdog support in GTDT driver fu.wei@linaro.org - 2016-11-16 15:00 +0100
  [PATCH v16 14/15] clocksource/drivers/arm_arch_timer: Add GTDT support for memory-mapped timer fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 14/15] clocksource/drivers/arm_arch_timer: Add GTDT  support for memory-mapped timer Mark Rutland <mark.rutland@arm.com> - 2016-11-18 21:30 +0100
      Re: [PATCH v16 14/15] clocksource/drivers/arm_arch_timer: Add GTDT  support for memory-mapped timer Fu Wei <fu.wei@linaro.org> - 2016-11-23 13:20 +0100
  [PATCH v16 08/15] clocksource/drivers/arm_arch_timer: Refactor arch_timer_needs_probing, and call it only if acpi disabled. fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 08/15] clocksource/drivers/arm_arch_timer: Refactor  arch_timer_needs_probing, and call it only if acpi disabled. Mark Rutland <mark.rutland@arm.com> - 2016-11-18 21:00 +0100
      Re: [PATCH v16 08/15] clocksource/drivers/arm_arch_timer: Refactor  arch_timer_needs_probing, and call it only if acpi disabled. Fu Wei <fu.wei@linaro.org> - 2016-11-21 15:40 +0100
  [PATCH v16 06/15] clocksource/drivers/arm_arch_timer: separate out arch_timer_uses_ppi init code to prepare for GTDT. fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 06/15] clocksource/drivers/arm_arch_timer: separate  out arch_timer_uses_ppi init code to prepare for GTDT. Mark Rutland <mark.rutland@arm.com> - 2016-11-18 20:40 +0100
      Re: [PATCH v16 06/15] clocksource/drivers/arm_arch_timer: separate  out arch_timer_uses_ppi init code to prepare for GTDT. Fu Wei <fu.wei@linaro.org> - 2016-11-21 10:50 +0100
  [PATCH v16 03/15] clocksource/drivers/arm_arch_timer: Improve printk relevant code fu.wei@linaro.org - 2016-11-16 15:00 +0100
  [PATCH v16 02/15] clocksource/drivers/arm_arch_timer: Add a new enum for spi type fu.wei@linaro.org - 2016-11-16 15:00 +0100
  [PATCH v16 09/15] clocksource/drivers/arm_arch_timer: Introduce some new structs to prepare for GTDT fu.wei@linaro.org - 2016-11-16 15:00 +0100
  [PATCH v16 05/15] clocksource/drivers/arm_arch_timer: fix a bug in arch_timer_register about arch_timer_uses_ppi fu.wei@linaro.org - 2016-11-16 15:00 +0100
    Re: [PATCH v16 05/15] clocksource/drivers/arm_arch_timer: fix a bug  in arch_timer_register about arch_timer_uses_ppi Mark Rutland <mark.rutland@arm.com> - 2016-11-18 20:00 +0100
      Re: [PATCH v16 05/15] clocksource/drivers/arm_arch_timer: fix a bug  in arch_timer_register about arch_timer_uses_ppi Fu Wei <fu.wei@linaro.org> - 2016-11-21 08:40 +0100
  Re: [PATCH v16 00/15] acpi, clocksource: add GTDT driver and GTDT  support in arm_arch_timer Xiongfeng Wang <wangxiongfeng2@huawei.com> - 2016-11-17 04:50 +0100
    Re: [PATCH v16 00/15] acpi, clocksource: add GTDT driver and GTDT  support in arm_arch_timer Xiongfeng Wang <wangxiongfeng2@huawei.com> - 2016-11-17 10:40 +0100
      Re: [PATCH v16 00/15] acpi, clocksource: add GTDT driver and GTDT  support in arm_arch_timer Fu Wei <fu.wei@linaro.org> - 2016-11-17 12:20 +0100
  Re: [PATCH v16 00/15] acpi, clocksource: add GTDT driver and GTDT  support in arm_arch_timer Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-11-17 10:50 +0100

csiph-web