Re: [PATCH v2] ACPI / LPIT: Add Low Power Idle Table (LPIT) support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 5, 2017 at 11:48 PM, Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> On Thu, 2017-10-05 at 22:56 +0200, Rafael J. Wysocki wrote:
>> On Thursday, October 5, 2017 10:43:33 PM CEST Srinivas Pandruvada
>> wrote:
>> >
>> > On Thu, 2017-10-05 at 21:39 +0300, Andy Shevchenko wrote:
>> > >
>> > > On Thu, Oct 5, 2017 at 9:16 PM, Srinivas Pandruvada
>> > > <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>> > > >
>> > > >
>> > > > Added functionality to read LPIT table, which provides:
>> > > >
>> > > > - Sysfs interface to read residency counters via
>> > > > /sys/devices/system/cpu/cpuidle/low_power_idle_cpu_residency_us
>> > > > /sys/devices/system/cpu/cpuidle/low_power_idle_system_residency
>> > > > _us
>> > > >
>> > > > Here the count "low_power_idle_cpu_residency_us" shows the time
>> > > > spent
>> > > > by CPU package in low power state. This is read via MSR
>> > > > interface,
>> > > > which
>> > > > points to MSR for PKG C10.
>> > > >
>> > > > Here the count "low_power_idle_system_residency_us" show the
>> > > > count
>> > > > the
>> > > > system was in low power state. This is read via MMIO interface.
>> > > > This
>> > > > is mapped to SLP_S0 residency on modern Intel systems. This
>> > > > residency
>> > > > is achieved only when CPU is in PKG C10 and all functional
>> > > > blocks
>> > > > are
>> > > > in low power state.
>> > > >
>> > > > It is possible that none of the above counters present or
>> > > > anyone of
>> > > > the
>> > > > counter present or all counters present.
>> > > >
>> > > > For example: On my Kabylake system both of the above counters
>> > > > present.
>> > > > After suspend to idle these counts updated and prints:
>> > > > 6916179
>> > > > 6998564
>> > > >
>> > > > This counter can be read by tools like turbostat to display. Or
>> > > > it
>> > > > can
>> > > > be used to debug, if modern systems are reaching desired low
>> > > > power
>> > > > state.
>> > > >
>> > > > - Provides an interface to read residency counter memory
>> > > > address
>> > > > This address can be used to get the base address of PMC memory
>> > > > mapped IO.
>> > > > This is utilized by intel_pmc_core driver to print more debug
>> > > > information.
>> > >
>> > > >
>> > > >
>> > > > +               switch (residency_info_mem.gaddr.bit_width) {
>> > > > +               case 8:
>> > > > +                       count =
>> > > > readb(residency_info_mem.iomem_addr);
>> > > > +                       break;
>> > > > +               case 16:
>> > > > +                       count =
>> > > > readw(residency_info_mem.iomem_addr);
>> > > > +                       break;
>> > > > +               case 32:
>> > > > +                       count =
>> > > > readl(residency_info_mem.iomem_addr);
>> > > > +                       break;
>> > > > +               case 64:
>> > > > +                       count =
>> > > > readq(residency_info_mem.iomem_addr);
>> > > > +                       break;
>> > > > +               default:
>> > > > +                       return -EINVAL;
>> > > > +               }
>> > >
>> > > I saw something very similar already under drivers/acpi. Can we
>> > > utilize it (split a helper out of it and re-use)?
>> > This functionality is probably not only for ACPI, but may be other
>> > parts of the kernel too. So if there is a common function then it
>> > can
>> > be more generic outside of ACPI.
>>
>> If the value of the field is a GAS, we can use the ACPICA's library
>> routine for reading from there I suppose.
>>
> Something like this? Only it the prototype is in a header file, with
> some defines for ACPI_USE_ALTERNATE_PROTOTYPE_acpi_os_read_memory

Yes, I guess that's what Andy had in mind. :-)

> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index db78d35..1b6ce24 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -663,6 +663,29 @@ acpi_status acpi_os_write_port(acpi_io_address
> port, u32 value, u32 width)
>
>  EXPORT_SYMBOL(acpi_os_write_port);
>
> +acpi_status acpi_os_read_iomem(void __iomem *virt_addr, u64 *value,
> u32 width)

That doesn't need to use acpi_status though, int should be fine.

> +{
> +
> +       switch (width) {
> +       case 8:
> +               *(u8 *) value = readb(virt_addr);
> +               break;
> +       case 16:
> +               *(u16 *) value = readw(virt_addr);
> +               break;
> +       case 32:
> +               *(u32 *) value = readl(virt_addr);
> +               break;
> +       case 64:
> +               *(u64 *) value = readq(virt_addr);
> +               break;
> +       default:
> +               return AE_ERROR;

And then return -EINVAL here.

> +       }
> +
> +       return AE_OK;

And 0 here.

> +}
> +
>  acpi_status
>  acpi_os_read_memory(acpi_physical_address phys_addr, u64 *value, u32
> width)
>  {
> @@ -684,22 +707,8 @@ acpi_os_read_memory(acpi_physical_address
> phys_addr, u64 *value, u32 width)
>         if (!value)
>                 value = &dummy;
>
> -       switch (width) {
> -       case 8:
> -               *(u8 *) value = readb(virt_addr);
> -               break;
> -       case 16:
> -               *(u16 *) value = readw(virt_addr);
> -               break;
> -       case 32:
> -               *(u32 *) value = readl(virt_addr);
> -               break;
> -       case 64:
> -               *(u64 *) value = readq(virt_addr);
> -               break;
> -       default:
> +       if (acpi_os_read_iomem(virt_addr, value, width) != AE_OK)
>                 BUG();

Here I would do

error = acpi_os_read_iomem(virt_addr, value, width);

BUG_ON(error);

> -       }
>
>         if (unmap)
>                 iounmap(virt_addr);
> diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
> index c66eb8f..6377e2d 100644
> --- a/include/acpi/acpiosxf.h
> +++ b/include/acpi/acpiosxf.h
> @@ -287,7 +287,10 @@ acpi_status acpi_os_write_port(acpi_io_address
> address, u32 value, u32 width);
>  /*
>   * Platform and hardware-independent physical memory interfaces
>   */
> +acpi_status acpi_os_read_iomem(void __iomem *virt_addr, u64 *value,
> u32 width);
> +
>  #ifndef ACPI_USE_ALTERNATE_PROTOTYPE_acpi_os_read_memory
> +acpi_status acpi_os_read_iomem(void __iomem *virt_addr, u64 *value,
> u32 width);
>  acpi_status
>  acpi_os_read_memory(acpi_physical_address address, u64 *value, u32
> width);
>  #endif
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux