Re: [PATCH v3 0/8] PM / ACPI: sleep: Additional changes related to suspend-to-idle
Hi Rafael On 02-Aug-19 4:03 PM, Rafael J. Wysocki wrote: Hi All, On top of the "Simplify the suspend-to-idle control flow" patch series posted previously: https://lore.kernel.org/lkml/71085220.z6FKkvYQPX@kreacher/ sanitize the suspend-to-idle flow even further. First off, decouple EC wakeup from the LPS0 _DSM processing (patch 1). Next, reorder the code to invoke LPS0 _DSM Functions 5 and 6 in the specification-compliant order with respect to suspending and resuming devices (patch 2). Finally, rearrange lps0_device_attach() (patch 3) and add a command line switch to prevent the LPS0 _DSM from being used. The v2 is because I found a (minor) bug in patch 1, decided to use a module parameter instead of a kernel command line option in patch 4. Also, there are 4 new patches: Patch 5: Switch the EC over to polling during "noirq" suspend and back during "noirq" resume. Patch 6: Eliminate acpi_sleep_no_ec_events(). Patch 7: Consolidate some EC code depending on PM_SLEEP. Patch 8: Add EC GPE dispatching debug message. The v3 is just a rearranged v2 so as to move the post sensitive patch (previous patch 2) to the end of the series. [After applying the full series the code is the same as before.] For easier testing, the series (along with some previous patches depended on by it) is available in the pm-s2idle-testing branch of the linux-pm.git tree at kernel.org: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/log/?h=pm-s2idle-testing Please refer to the changelogs for details. I have tested both pm-s2idle-testing and pm-s2idle-rework branches including recently introduced commit "PM: suspend: Fix platform_suspend_prepare_noirq()". Works fine for me on Ice Lake platform. Acked-by: Rajneesh Bhardwaj Tested-by: Rajneesh Bhardwaj Thanks, Rafael
Re: [PATCH 2/2] powercap/rapl: Add Ice Lake NNPI support to RAPL driver
Hi Rafael On 28-Jun-19 3:32 AM, Rafael J. Wysocki wrote: On Friday, June 14, 2019 10:05:23 AM CEST Rajneesh Bhardwaj wrote: Enables support for ICL-NNPI, which is a neural network processor for deep learning inference. From RAPL point of view it is same as Ice Lake Mobile processor. Cc: "Rafael J. Wysocki" Cc: linux...@vger.kernel.org Link: https://lkml.org/lkml/2019/6/5/1034 Signed-off-by: Rajneesh Bhardwaj --- drivers/powercap/intel_rapl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c index 4347f15165f8..431c8c8bdf07 100644 --- a/drivers/powercap/intel_rapl.c +++ b/drivers/powercap/intel_rapl.c @@ -1157,6 +1157,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = { INTEL_CPU_FAM6(KABYLAKE_DESKTOP,rapl_defaults_core), INTEL_CPU_FAM6(CANNONLAKE_MOBILE, rapl_defaults_core), INTEL_CPU_FAM6(ICELAKE_MOBILE, rapl_defaults_core), + INTEL_CPU_FAM6(ICELAKE_NNPI,rapl_defaults_core), INTEL_CPU_FAM6(ATOM_SILVERMONT, rapl_defaults_byt), INTEL_CPU_FAM6(ATOM_AIRMONT,rapl_defaults_cht), It is in my queue, but I get build errors when I try to apply it. I guess the definition of ICELAKE_NNPI is not there in the Linus' tree yet. The dependent patch is now available in mainline, so can you please apply this one? https://github.com/torvalds/linux/blob/bed38c3e2dca01b358a62b5e73b46e875742fd75/arch/x86/include/asm/intel-family.h#L59 Thanks Rajneesh
Re: [PATCH] platform/x86: intel_pmc_core: Add ICL-NNPI support to PMC Core
Hi Andy On 29-Jun-19 6:48 PM, Andy Shevchenko wrote: On Fri, Jun 14, 2019 at 11:14 AM Rajneesh Bhardwaj wrote: Ice Lake Neural Network Processor for deep learning inference a.k.a. ICL-NNPI can re-use Ice Lake Mobile regmap to enable Intel PMC Core driver on it. This will be postponed till next cycle since the CPU model will not appear before merge window. So, please, resend than (I guess somewhat in a month). The dependent CPUID Patch is now upstream, https://github.com/torvalds/linux/blob/bed38c3e2dca01b358a62b5e73b46e875742fd75/arch/x86/include/asm/intel-family.h#L59 so can you please apply this one? Thank you Rajneesh Cc: Darren Hart Cc: Andy Shevchenko Cc: platform-driver-...@vger.kernel.org Link: https://lkml.org/lkml/2019/6/5/1034 Signed-off-by: Rajneesh Bhardwaj --- drivers/platform/x86/intel_pmc_core.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 1d902230ba61..be6cda89dcf5 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -815,6 +815,7 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = { INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map), INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map), INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map), + INTEL_CPU_FAM6(ICELAKE_NNPI, icl_reg_map), {} }; -- 2.17.1
Re: [PATCH 2/2] powercap/rapl: Add Ice Lake NNPI support to RAPL driver
Hi Rafael, On 05-Jul-19 3:43 PM, Rafael J. Wysocki wrote: On Friday, June 28, 2019 10:21:41 AM CEST Bhardwaj, Rajneesh wrote: On 28-Jun-19 3:32 AM, Rafael J. Wysocki wrote: On Friday, June 14, 2019 10:05:23 AM CEST Rajneesh Bhardwaj wrote: Enables support for ICL-NNPI, which is a neural network processor for deep learning inference. From RAPL point of view it is same as Ice Lake Mobile processor. Cc: "Rafael J. Wysocki" Cc: linux...@vger.kernel.org Link: https://lkml.org/lkml/2019/6/5/1034 Signed-off-by: Rajneesh Bhardwaj --- drivers/powercap/intel_rapl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c index 4347f15165f8..431c8c8bdf07 100644 --- a/drivers/powercap/intel_rapl.c +++ b/drivers/powercap/intel_rapl.c @@ -1157,6 +1157,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = { INTEL_CPU_FAM6(KABYLAKE_DESKTOP,rapl_defaults_core), INTEL_CPU_FAM6(CANNONLAKE_MOBILE, rapl_defaults_core), INTEL_CPU_FAM6(ICELAKE_MOBILE, rapl_defaults_core), + INTEL_CPU_FAM6(ICELAKE_NNPI,rapl_defaults_core), INTEL_CPU_FAM6(ATOM_SILVERMONT, rapl_defaults_byt), INTEL_CPU_FAM6(ATOM_AIRMONT,rapl_defaults_cht), It is in my queue, but I get build errors when I try to apply it. I guess the definition of ICELAKE_NNPI is not there in the Linus' tree yet. Yes, though https://www.spinics.net/lists/linux-tip-commits/msg48978.html is accepted by Thomas already. It is not in 5.2-rc7, though, AFAICS. The patch is still in my queue and waiting. The CPUID patch on which this change depends is upstream now. https://github.com/torvalds/linux/blob/bed38c3e2dca01b358a62b5e73b46e875742fd75/arch/x86/include/asm/intel-family.h#L59 Thank you Rajneesh
Re: [PATCH 2/2] powercap/rapl: Add Ice Lake NNPI support to RAPL driver
On 28-Jun-19 3:32 AM, Rafael J. Wysocki wrote: On Friday, June 14, 2019 10:05:23 AM CEST Rajneesh Bhardwaj wrote: Enables support for ICL-NNPI, which is a neural network processor for deep learning inference. From RAPL point of view it is same as Ice Lake Mobile processor. Cc: "Rafael J. Wysocki" Cc: linux...@vger.kernel.org Link: https://lkml.org/lkml/2019/6/5/1034 Signed-off-by: Rajneesh Bhardwaj --- drivers/powercap/intel_rapl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c index 4347f15165f8..431c8c8bdf07 100644 --- a/drivers/powercap/intel_rapl.c +++ b/drivers/powercap/intel_rapl.c @@ -1157,6 +1157,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = { INTEL_CPU_FAM6(KABYLAKE_DESKTOP,rapl_defaults_core), INTEL_CPU_FAM6(CANNONLAKE_MOBILE, rapl_defaults_core), INTEL_CPU_FAM6(ICELAKE_MOBILE, rapl_defaults_core), + INTEL_CPU_FAM6(ICELAKE_NNPI,rapl_defaults_core), INTEL_CPU_FAM6(ATOM_SILVERMONT, rapl_defaults_byt), INTEL_CPU_FAM6(ATOM_AIRMONT,rapl_defaults_cht), It is in my queue, but I get build errors when I try to apply it. I guess the definition of ICELAKE_NNPI is not there in the Linus' tree yet. Yes, though https://www.spinics.net/lists/linux-tip-commits/msg48978.html is accepted by Thomas already.
Re: [PATCH 2/2] powercap/rapl: Add Ice Lake NNPI support to RAPL driver
Hi Rafael On 14-Jun-19 1:09 PM, Rajneesh Bhardwaj wrote: Enables support for ICL-NNPI, neural network processor for deep learning inference. From RAPL point of view it is same as Ice Lake Mobile processor. Cc: "Rafael J. Wysocki" Cc: linux...@vger.kernel.org Link: https://lkml.org/lkml/2019/6/5/1034 Signed-off-by: Rajneesh Bhardwaj --- drivers/powercap/intel_rapl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c index 4347f15165f8..19d470c73b6e 100644 --- a/drivers/powercap/intel_rapl.c +++ b/drivers/powercap/intel_rapl.c @@ -1156,7 +1156,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = { INTEL_CPU_FAM6(KABYLAKE_MOBILE, rapl_defaults_core), INTEL_CPU_FAM6(KABYLAKE_DESKTOP,rapl_defaults_core), INTEL_CPU_FAM6(CANNONLAKE_MOBILE, rapl_defaults_core), - INTEL_CPU_FAM6(ICELAKE_MOBILE, rapl_defaults_core), + INTEL_CPU_FAM6(ICELAKE_NNPI,rapl_defaults_core), Sorry, i sent the wrong patch, please ignore this one. I will send another one. Thanks. INTEL_CPU_FAM6(ATOM_SILVERMONT, rapl_defaults_byt), INTEL_CPU_FAM6(ATOM_AIRMONT,rapl_defaults_cht),
Re: [Patch v2] x86/cpu: Add Ice Lake NNPI to Intel family
On 12-Jun-19 10:38 PM, Andy Shevchenko wrote: On Wed, Jun 12, 2019 at 09:33:30AM -0700, Dave Hansen wrote: On 6/12/19 9:29 AM, Andy Shevchenko wrote: What I'm talking is a consistency among suffixes. If there is a real abbreviation (NNPI) which anybody can google, There is and you can. :) Good, I have no objections. Great, thanks Andy and Dave for your valuable comments. I see that Thomas has accepted this one, thanks a lot, Thomas! https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=61caa8621b9979a78b04e353ab2ee44a47ef7a62=1
Re: [PATCH] x86/cpu: Add Icelake-NNPI to Intel family
Hi Andy On 04-Jun-19 9:39 PM, Andy Shevchenko wrote: On Thu, May 30, 2019 at 06:08:27PM +0530, Rajneesh Bhardwaj wrote: Add the CPUID model number of Icelake Neural Network Processor for Deep I believe we spell "Ice Lake". I referred to https://patchwork.kernel.org/patch/10812551/ , https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6e394376ee89233508fa21d006546357f8efee31 and many others where it mentioned Icelake. I am fine to change it the way you are suggesting, please confirm if its still needed and i will send a v2. Thank you. Learning Inference (ICL-NNPI) to the Intel family list. Icelake NNPI uses Ditto. model number 0x9D and this will be documented in a future version of Intel Software Development Manual. Cc: Borislav Petkov Cc: Dave Hansen Cc: Andy Shevchenko Cc: "H. Peter Anvin" Cc: Ingo Molnar Cc: Kan Liang Cc: Peter Zijlstra Cc: platform-driver-...@vger.kernel.org Cc: Qiuxu Zhuo Cc: Srinivas Pandruvada Cc: Len Brown Cc: Thomas Gleixner Cc: x86-ml Cc: Linux PM Signed-off-by: Rajneesh Bhardwaj --- arch/x86/include/asm/intel-family.h | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h index 9f15384c504a..087de5d3b93a 100644 --- a/arch/x86/include/asm/intel-family.h +++ b/arch/x86/include/asm/intel-family.h @@ -53,6 +53,7 @@ #define INTEL_FAM6_CANNONLAKE_MOBILE 0x66 #define INTEL_FAM6_ICELAKE_MOBILE 0x7E +#define INTEL_FAM6_ICELAKE_NNPI0x9D /* "Small Core" Processors (Atom) */ -- 2.17.1
Re: [PATCH] platform/x86: intel_pmc_core: Mark local function static
Sending again as i got delivery failure notification from the mailing list for some reason. On 05-Apr-19 8:15 PM, Andy Shevchenko wrote: On Wed, Mar 27, 2019 at 2:47 PM Bhardwaj, Rajneesh wrote: On 27-Mar-19 1:59 AM, Guenter Roeck wrote: 0day reports: drivers/platform/x86/intel_pmc_core.c:833:5: sparse: symbol 'quirk_xtal_ignore' was not declared. Should it be static? Looks good to me. Please, give a corresponding tag. Meanwhile I will apply it to my review and testing queue for robots to play with, thanks! Sure. Acked-by: Rajneesh Bhardwaj Mark the function static since it is indeed only called locally. Cc: Rajneesh Bhardwaj Fixes: 238f9c11351f ("platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown") Signed-off-by: Guenter Roeck --- drivers/platform/x86/intel_pmc_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index f2c621b55f49..9908d233305e 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -828,7 +828,7 @@ static const struct pci_device_id pmc_pci_ids[] = { * the platform BIOS enforces 24Mhx Crystal to shutdown * before PMC can assert SLP_S0#. */ -int quirk_xtal_ignore(const struct dmi_system_id *id) +static int quirk_xtal_ignore(const struct dmi_system_id *id) { struct pmc_dev *pmcdev = u32 value;
Re: [PATCH] platform/x86: intel_pmc_core: Mark local function static
On 27-Mar-19 1:59 AM, Guenter Roeck wrote: 0day reports: drivers/platform/x86/intel_pmc_core.c:833:5: sparse: symbol 'quirk_xtal_ignore' was not declared. Should it be static? Looks good to me. Mark the function static since it is indeed only called locally. Cc: Rajneesh Bhardwaj Fixes: 238f9c11351f ("platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown") Signed-off-by: Guenter Roeck --- drivers/platform/x86/intel_pmc_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index f2c621b55f49..9908d233305e 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -828,7 +828,7 @@ static const struct pci_device_id pmc_pci_ids[] = { * the platform BIOS enforces 24Mhx Crystal to shutdown * before PMC can assert SLP_S0#. */ -int quirk_xtal_ignore(const struct dmi_system_id *id) +static int quirk_xtal_ignore(const struct dmi_system_id *id) { struct pmc_dev *pmcdev = u32 value;
Re: [PATCH 1/2] platform/x86: intel_pmc_core: Convert to a platform_driver
Hi Rajat On 23-Mar-19 6:00 AM, Rajat Jain wrote: Hi Rajneesh, On Fri, Mar 22, 2019 at 12:56 PM Bhardwaj, Rajneesh wrote: Some suggestions below On 18-Mar-19 8:36 PM, Rajat Jain wrote: On Sat, Mar 16, 2019 at 1:30 AM Rajneesh Bhardwaj wrote: On Wed, Mar 13, 2019 at 03:21:23PM -0700, Rajat Jain wrote: Convert the intel_pmc_core driver to a platform driver. There is no functional change. Some code that tries to determine what kind of CPU this is, has been moved code is moved from pmc_core_probe() to Possible typo here. Ummm, you mean grammar error I guess? Sure, I will rephrase. pmc_core_init(). Signed-off-by: Rajat Jain Thanks for sending this. This is certainly useful to support suspend-resume functionality for this driver which is otherwise only possible with PM notifiers otherwise and that is not desirable. Initially this was a PCI driver and after design discussion it was converted to module. I would like to consult Andy and Srinivas for their opinion about binding it to actual platform bus instead of the virtual bus as in its current form. In one of the internal versions, we used a known acpi PNP HID. Sure, if there is an established ACPI PNP HID, then we could bind it using that, on platforms where we are still developing BIOS / coreboot. However, this might not be possible for shipping systems (Kabylake / skylake) where there is no plan to change the BIOS. In one of our internal patches, i had used HID of power engine plugin. IIRC, During my testing it was working on KBL, CNL with UEFI BIOS but i highly recommend testing it. ---8<8<- +static const struct acpi_device_id pmc_acpi_ids[] = { + {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/ + { } }; We do not have this device in any of our ACPI tables today. If Intel can confirm that this is a well known HID to be used for attaching this driver, we can start putting it on our platform's ACPI going forward (Whiskeylake, Cometlake, Cannonlake, Icelake ...). But I believe we also need to have this driver attach with the device on older platforms (Skylake, Kabylake, Amberlake) that are already shipping, and running a Non UEFI BIOS (that may not have this HID since it is not published). Currently the intel_pmc_core driver attaches itself to the following table of CPU families, without regard to whether it has that HID in the ACPI or not: static const struct x86_cpu_id intel_pmc_core_ids[] = { INTEL_CPU_FAM6(SKYLAKE_MOBILE, spt_reg_map), INTEL_CPU_FAM6(SKYLAKE_DESKTOP, spt_reg_map), INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map), INTEL_CPU_FAM6(KABYLAKE_DESKTOP, spt_reg_map), INTEL_CPU_FAM6(CANNONLAKE_MOBILE, cnp_reg_map), INTEL_CPU_FAM6(ICELAKE_MOBILE, icl_reg_map), {} }; In the past i tried one hybrid approach i.e. PCI and Platform driver at the same time. Based on that, i feel that this idea of spilling probe like this may not be the best option. The ACPI CID that i suggested is available on most Intel Core Platforms that i have worked on and i can help you in verifying it with UEFI BIOS if you want. Meanwhile, please see this https://patchwork.kernel.org/patch/9806565/ it gives some background about this ACPI ID and also points to the LPIT spec. So to avoid a regression, I suggest that we still maintain the above table (may be eliminate few entries) and always attach if the CPU is among the table, and if the CPU is not among the table, use the ACPI HID to attach. I propose to attach to at least Skylake and Kabylake systems using the table above, and for Canonlake and Icelake and newer, we can rely on BIOS providing the ACPI HID. Of course I do not know if all non-Google Canonlake/Icelake platforms will have this HID in their BIOS. If we are not sure, we can include Canonlake and Icelake also in that list, an. Please let me know what do you think. If Coreboot firmware can not be updated for the shipping devices, then can Chromium kernel take the suggested deviation which i think gets updated OTA periodically? For upstream, I am waiting to hear from Rafael, Andi, David and Srinivas for their inputs. Thanks, Rajat -builtin_pci_driver(intel_pmc_core_driver); +static struct platform_driver pmc_plat_driver = { + .remove = pmc_plat_remove, + .probe = pmc_plat_probe, + .driver = { + .name = "pmc_core_driver", + .acpi_match_table = ACPI_PTR(pmc_acpi_ids), + }, +}; --- This is rebased off git://git.infradead.org/linux-platform-drivers-x86.git/for-next drivers/platform/x86/intel_pmc_core.c | 93 --- 1 file changed, 68 insertions(+), 25 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index f2c621b55f49..55578d07610c 100644 --- a/drivers/platform/x86/intel_pmc_core.c
Re: [PATCH v3 0/5] ICL support and other enhancements for PMC Core
On 21-Feb-19 7:55 PM, Andy Shevchenko wrote: On Thu, Feb 14, 2019 at 1:56 PM Rajneesh Bhardwaj wrote: Changes in v3: * Dropped reference to coreboot project as suggested by Thomas and Boris. * Rebased onto "for-next" branch of pdx86 tree and dropped previously accepted five patches from v2 of this series since they are already present there. * Fixed checkpatch complains about 75 char limit for commit messages. Changes in v2: * Addressed review comments from Thomas * Added tags revieved * Folded in SHA1 suggestions from Stephen Rothwell, though Andy might want to fix it via rebasing * Rebased and tested with Linux v5.0.0-rc6 This series: - Adds ICL U/Y CPUID to intel-family.h - Enables PMC driver for ICL - Introduces a new "package cstate show" feature - Fixes a customer issue related to S0ix on latest HP laptops - Fixes some minor bugs Pushed to my review and testing queue, thanks! Thank you! Rajneesh Bhardwaj (5): x86/cpu: Add Icelake to Intel family platform/x86: intel_pmc_core: Convert to INTEL_CPU_FAM6 macro platform/x86: intel_pmc_core: Add ICL platform support platform/x86: intel_pmc_core: Add Package cstates residency info platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown arch/x86/include/asm/intel-family.h | 2 + drivers/platform/x86/intel_pmc_core.c | 144 ++ drivers/platform/x86/intel_pmc_core.h | 10 ++ 3 files changed, 135 insertions(+), 21 deletions(-) -- 2.17.1
Re: [PATCH] platform/x86: intel_pmc_core: Avoid a u32 overflow
On 16-Feb-19 5:49 AM, Rajat Jain wrote: The register (SLP_S0_RES) at offset slp_s0_offset is a 32 bit register. The pmc_core_adjust_slp_s0_step() could overflow the u32 value while returning it after adjusting the step. Thus change to u64, this is already accounted for in debugfs attribute (that wants to output a 64 bit value). Signed-off-by: Rajat Jain Acked-by: Rajneesh Bhardwaj --- drivers/platform/x86/intel_pmc_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 22dbf115782e..f90f4dd25151 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -328,9 +328,9 @@ static inline void pmc_core_reg_write(struct pmc_dev *pmcdev, int writel(val, pmcdev->regbase + reg_offset); } -static inline u32 pmc_core_adjust_slp_s0_step(u32 value) +static inline u64 pmc_core_adjust_slp_s0_step(u32 value) { - return value * SPT_PMC_SLP_S0_RES_COUNTER_STEP; + return (u64)value * SPT_PMC_SLP_S0_RES_COUNTER_STEP; } static int pmc_core_dev_state_get(void *data, u64 *val)
Re: [PATCH v2 00/10] ICL support and other enhancements for PMC Core
On 13-Feb-19 10:58 PM, Andy Shevchenko wrote: On Wed, Feb 13, 2019 at 5:50 PM Bhardwaj, Rajneesh wrote: On 13-Feb-19 9:03 PM, Andy Shevchenko wrote: On Wed, Feb 13, 2019 at 5:08 PM Rajneesh Bhardwaj wrote: This patch series provides Icelake support for PMC Core driver and while doing so it introduces the Icelake Mobile to intel-family.h as per the CPUID from below Coreboot link https://github.com/coreboot/coreboot/blob/5ebcea33cd358bc5bccaa156b13a6ef25df6/src/soc/intel/common/block/include/intelblocks/mp_init.h and provides some fixes and enhancements to the driver. It's not applicable to my tree. Hi Andy, I could apply and test in on latest upstream so i think its probably because you have applied 5 patches from v1 on your tree. https://github.com/dvhart/linux-pdx86/commits/for-next You need to use latest tip of the subsystem tree always. Ok. I sent the whole series again since https://patchwork.kernel.org/patch/10791893 referred to a wrong commit for Fixes:. If you want to address that by rebasing then can you please just consider patches 6-10. We don't do rebasing for published changes. Only in rear cases when otherwise would be worse. Darren didn't respond to my question what he thinks about this case, but at least it's not related to the code itself which, in my opinion, decreases a severity. Thanks Andy. So if i understand correctly, you are suggesting that i should ignore those patches that made to "for-next" branch already? So should i just send a v3 comprising of remaining 5 patches and of course, addressing recent review comments? Please let me know if there is any other issue?
Re: [PATCH v2 06/10] x86/cpu: Add Icelake to Intel family
On 13-Feb-19 10:10 PM, Dave Hansen wrote: On 2/13/19 8:35 AM, Bhardwaj, Rajneesh wrote: I sure did, perhaps it wasn't clear in my response. I can remove coreboot link in next version but please clarify whether i should keep other link that i mentioned or just keep the commit without any link? I think we're hearing loud and clear from the maintainers that they prefer *public*, official documentation from Intel to back up our patches. Barring that, they'd rather have no link than a link to some other random project. Thank you Dave for the clarification. Hi Borislav, I realize that the link i mentioned is not public. Apologies for creating confusion around that and for the previous top posting. I will drop references to the coreboot link too in the next version.
Re: [PATCH v2 06/10] x86/cpu: Add Icelake to Intel family
I sure did, perhaps it wasn't clear in my response. I can remove coreboot link in next version but please clarify whether i should keep other link that i mentioned or just keep the commit without any link? On 13-Feb-19 9:34 PM, Borislav Petkov wrote: On Wed, Feb 13, 2019 at 09:13:01PM +0530, Bhardwaj, Rajneesh wrote: Icelake related information is available here https://www.intel.in/content/www/in/en/design/products-and-solutions/processors-and-chipsets/ice-lake/overview.html but it may require a login either with Intel or a Partner ID. The CPUID mentioned in this document is 0x706E0 which is same as per this coreboot link. You did not read what I wrote.
Re: [PATCH v2 00/10] ICL support and other enhancements for PMC Core
On 13-Feb-19 9:03 PM, Andy Shevchenko wrote: On Wed, Feb 13, 2019 at 5:08 PM Rajneesh Bhardwaj wrote: This patch series provides Icelake support for PMC Core driver and while doing so it introduces the Icelake Mobile to intel-family.h as per the CPUID from below Coreboot link https://github.com/coreboot/coreboot/blob/5ebcea33cd358bc5bccaa156b13a6ef25df6/src/soc/intel/common/block/include/intelblocks/mp_init.h and provides some fixes and enhancements to the driver. It's not applicable to my tree. Hi Andy, I could apply and test in on latest upstream so i think its probably because you have applied 5 patches from v1 on your tree. https://github.com/dvhart/linux-pdx86/commits/for-next I sent the whole series again since https://patchwork.kernel.org/patch/10791893 referred to a wrong commit for Fixes:. If you want to address that by rebasing then can you please just consider patches 6-10. Please let me know if there is any other issue? Changes in v2: * Addressed review comments from Thomas * Added tags revieved * Folded in SHA1 suggestions from Stephen Rothwell, though Andy might want to fix it via rebasing * Rebased and tested with Linux v5.0.0-rc6 This series: - Adds ICL U/Y CPUID to intel-family.h - Enables PMC driver for ICL - Introduces a new "package cstate show" feature - Fixes a customer issue related to S0ix on latest HP laptops - Fixes some minor bugs Rajneesh Bhardwaj (10): platform/x86: intel_pmc_core: Handle CFL regmap properly platform/x86: intel_pmc_core: Fix PCH IP sts reading platform/x86: intel_pmc_core: Fix PCH IP name platform/x86: intel_pmc_core: Fix file permissions for ltr_show platform/x86: intel_pmc_core: Include Reserved IP for LTR x86/cpu: Add Icelake to Intel family platform/x86: intel_pmc_core: Convert to INTEL_CPU_FAM6 macro platform/x86: intel_pmc_core: Add ICL platform support platform/x86: intel_pmc_core: Add Package cstates residency info platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown arch/x86/include/asm/intel-family.h | 2 + drivers/platform/x86/intel_pmc_core.c | 155 +- drivers/platform/x86/intel_pmc_core.h | 14 ++- 3 files changed, 145 insertions(+), 26 deletions(-) -- 2.17.1
Re: [PATCH v2 06/10] x86/cpu: Add Icelake to Intel family
On 13-Feb-19 8:53 PM, Borislav Petkov wrote: On Wed, Feb 13, 2019 at 08:38:06PM +0530, Rajneesh Bhardwaj wrote: Add CPUID of Icelake (ICL) mobile processors to Intel family list. The information related to ICL CPUID is referenced from below Coreboot project link. https://github.com/coreboot/coreboot/blob/5ebcea33cd358bc5bccaa156b13a6ef25df6/src/soc/intel/common/block/include/intelblocks/mp_init.h I believe tglx was suggesting you should drop this link. If you can't reference an official document, then you don't need any reference. Icelake related information is available here https://www.intel.in/content/www/in/en/design/products-and-solutions/processors-and-chipsets/ice-lake/overview.html but it may require a login either with Intel or a Partner ID. The CPUID mentioned in this document is 0x706E0 which is same as per this coreboot link.
Re: [PATCH v2 10/10] platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown
On 13-Feb-19 8:51 PM, mario.limoncie...@dell.com wrote: -Original Message- From: platform-driver-x86-ow...@vger.kernel.org On Behalf Of Rajneesh Bhardwaj Sent: Wednesday, February 13, 2019 9:08 AM To: platform-driver-...@vger.kernel.org Cc: dvh...@infradead.org; a...@infradead.org; linux-kernel@vger.kernel.org; Rajneesh Bhardwaj Subject: [PATCH v2 10/10] platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown [EXTERNAL EMAIL] On some platforms such as HP Elite-x2-1013-g3, the platform BIOS enforces XTAL to remain off before S0ix state can be achieved. This may not be optimum when we want to enable use cases like Low Power Audio, Wake on Voice etc which always need 24mhz clock. This introduces a new quirk to allow S0ix entry when all other conditions except for XTAL clock are good on a given platform. The extra power consumed by XTAL clock is about 2mw but it saves much more platform power compared to the system that remains in just PC10. I wonder are there really any use cases for 24 mhz clock "needing" to stay enabled on Linux over a S0ix cycle and factor into the S0ix state decision? Is it perhaps better to set this as default behavior and quirk situations that it may not be needed. Hi Mario, I agree but the PMC default settings are determined by the platform BIOS. Some vendors may want to support WakeOnVoice and other use cases where a dependent clock from XTAL may be required while others might just not care. For this particular machine, the BIOS runs some special asl code that handles these settings for Windows and this is a deviation from normal. Thus we prefer a quirk for this and will monitor similar reports from other vendors too on other SoCs as well before we make this default in driver. Link: https://bit.ly/2UmnrFf Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201579 Tested-by: "David E. Box" Reported-and-tested-by: russianneuromancer Signed-off-by: Rajneesh Bhardwaj --- drivers/platform/x86/intel_pmc_core.c | 34 +++ drivers/platform/x86/intel_pmc_core.h | 5 2 files changed, 39 insertions(+) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 4e7aa1711148..a27574e3e868 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -151,6 +152,7 @@ static const struct pmc_reg_map spt_reg_map = { .pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET, .pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT, .ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED, + .pm_vric1_offset = SPT_PMC_VRIC1_OFFSET, }; /* Cannonlake: PGD PFET Enable Ack Status Register(s) bitmap */ @@ -821,6 +823,37 @@ static const struct pci_device_id pmc_pci_ids[] = { { 0, }, }; +/* + * This quirk can be used on those platforms where + * the platform BIOS enforces 24Mhx Crystal to shutdown + * before PMC can assert SLP_S0#. + */ +int quirk_xtal_ignore(const struct dmi_system_id *id) +{ + struct pmc_dev *pmcdev = + u32 value; + + value = pmc_core_reg_read(pmcdev, pmcdev->map->pm_vric1_offset); + /* 24MHz Crystal Shutdown Qualification Disable */ + value |= SPT_PMC_VRIC1_XTALSDQDIS; + /* Low Voltage Mode Enable */ + value &= ~SPT_PMC_VRIC1_SLPS0LVEN; + pmc_core_reg_write(pmcdev, pmcdev->map->pm_vric1_offset, value); + return 0; +} + +static const struct dmi_system_id pmc_core_dmi_table[] = { + { + .callback = quirk_xtal_ignore, + .ident = "HP Elite x2 1013 G3", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "HP"), + DMI_MATCH(DMI_PRODUCT_NAME, "HP Elite x2 1013 G3"), + }, + }, + {} +}; + static int __init pmc_core_probe(void) { struct pmc_dev *pmcdev = @@ -862,6 +895,7 @@ static int __init pmc_core_probe(void) return err; } + dmi_check_system(pmc_core_dmi_table); pr_info(" initialized\n"); return 0; } diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h index 6f1b64808075..88d9c0653a5f 100644 --- a/drivers/platform/x86/intel_pmc_core.h +++ b/drivers/platform/x86/intel_pmc_core.h @@ -25,6 +25,7 @@ #define SPT_PMC_MTPMC_OFFSET 0x20 #define SPT_PMC_MFPMC_OFFSET 0x38 #define SPT_PMC_LTR_IGNORE_OFFSET 0x30C +#define SPT_PMC_VRIC1_OFFSET 0x31c #define SPT_PMC_MPHY_CORE_STS_0 0x1143 #define SPT_PMC_MPHY_CORE_STS_1 0x1142 #define SPT_PMC_MPHY_COM_STS_00x1155 @@ -136,6 +137,9 @@ enum ppfear_regs { #define SPT_PMC_BIT_MPHY_CMN_LANE2BIT(2) #define SPT_PMC_BIT_MPHY_CMN_LANE3BIT(3) +#define SPT_PMC_VRIC1_SLPS0LVENBIT(13) +#define
Re: [PATCH 08/10] platform/x86: intel_pmc_core: Add ICL platform support
On 12-Feb-19 3:48 PM, Andy Shevchenko wrote: On Tue, Feb 12, 2019 at 11:46 AM Anshuman Gupta wrote: On Fri, Feb 01, 2019 at 01:02:32PM +0530, Rajneesh Bhardwaj wrote: Icelake can resue most of the CNL PCH IPs as they are mostly similar. This patch enables the PMC Core driver for ICL family. It also addresses few other minor issues like upper case conversions and some tab alignments. Cc: "David E. Box" Cc: Srinivas Pandruvada Signed-off-by: Rajneesh Bhardwaj Acked-and-tested-by: Thanks. Can you clarify what patches had been tested? As far as I can understand you can't test this one before applying previous seven patches. Rajneesh, when you will be about to send the rest, don't forget to append the received tags. Sure Andy. I have sent the v2, incorporating tags and other suggestions that came over last two weeks. --- drivers/platform/x86/intel_pmc_core.c | 59 +-- drivers/platform/x86/intel_pmc_core.h | 4 ++ 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index d3752d75075b..400946b7a3b5 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -166,25 +166,26 @@ static const struct pmc_bit_map cnp_pfear_map[] = { {"SDX", BIT(4)}, {"SPE", BIT(5)}, {"Fuse",BIT(6)}, - {"Res_23", BIT(7)}, + /* Reserved for Cannonlake but valid for Icelake */ + {"SBR8",BIT(7)}, {"CSME_FSC",BIT(0)}, {"USB3_OTG",BIT(1)}, {"EXI", BIT(2)}, {"CSE", BIT(3)}, - {"csme_kvm",BIT(4)}, - {"csme_pmt",BIT(5)}, - {"csme_clink", BIT(6)}, - {"csme_ptio", BIT(7)}, - - {"csme_usbr", BIT(0)}, - {"csme_susram", BIT(1)}, - {"csme_smt1", BIT(2)}, + {"CSME_KVM",BIT(4)}, + {"CSME_PMT",BIT(5)}, + {"CSME_CLINK", BIT(6)}, + {"CSME_PTIO", BIT(7)}, + + {"CSME_USBR", BIT(0)}, + {"CSME_SUSRAM", BIT(1)}, + {"CSME_SMT1", BIT(2)}, {"CSME_SMT4", BIT(3)}, - {"csme_sms2", BIT(4)}, - {"csme_sms1", BIT(5)}, - {"csme_rtc",BIT(6)}, - {"csme_psf",BIT(7)}, + {"CSME_SMS2", BIT(4)}, + {"CSME_SMS1", BIT(5)}, + {"CSME_RTC",BIT(6)}, + {"CSME_PSF",BIT(7)}, {"SBR0",BIT(0)}, {"SBR1",BIT(1)}, @@ -209,6 +210,20 @@ static const struct pmc_bit_map cnp_pfear_map[] = { {"HDA_PGD4",BIT(2)}, {"HDA_PGD5",BIT(3)}, {"HDA_PGD6",BIT(4)}, + /* Reserved for Cannonlake but valid for Icelake */ + {"PSF6",BIT(5)}, + {"PSF7",BIT(6)}, + {"PSF8",BIT(7)}, + + /* Icelake generation onwards only */ + {"RES_65", BIT(0)}, + {"RES_66", BIT(1)}, + {"RES_67", BIT(2)}, + {"TAM", BIT(3)}, + {"GBETSN", BIT(4)}, + {"TBTLSX", BIT(5)}, + {"RES_71", BIT(6)}, + {"RES_72", BIT(7)}, {} }; @@ -290,6 +305,8 @@ static const struct pmc_bit_map cnp_ltr_show_map[] = { {"ISH", CNP_PMC_LTR_ISH}, {"UFSX2", CNP_PMC_LTR_UFSX2}, {"EMMC",CNP_PMC_LTR_EMMC}, + /* Reserved for Cannonlake but valid for Icelake */ + {"WIGIG", ICL_PMC_LTR_WIGIG}, /* Below two cannot be used for LTR_IGNORE */ {"CURRENT_PLATFORM",CNP_PMC_LTR_CUR_PLT}, {"AGGREGATED_SYSTEM", CNP_PMC_LTR_CUR_ASLT}, @@ -311,6 +328,21 @@ static const struct pmc_reg_map cnp_reg_map = { .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED, }; +static const struct pmc_reg_map icl_reg_map = { + .pfear_sts = cnp_pfear_map, + .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET, + .slps0_dbg_maps = cnp_slps0_dbg_maps, + .ltr_show_sts = cnp_ltr_show_map, + .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET, + .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET, + .regmap_length = CNP_PMC_MMIO_REG_LEN, + .ppfear0_offset = CNP_PMC_HOST_PPFEAR0A, + .ppfear_buckets = ICL_PPFEAR_NUM_ENTRIES, + .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET, + .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT, + .ltr_ignore_max = ICL_NUM_IP_IGN_ALLOWED, +}; + static inline u8 pmc_core_reg_read_byte(struct pmc_dev *pmcdev, int offset) { return readb(pmcdev->regbase + offset); @@ -740,6 +772,7 @@ static const struct x86_cpu_id intel_pmc_core_ids[] = { INTEL_CPU_FAM6(KABYLAKE_MOBILE, spt_reg_map),
Re: linux-next: Fixes tag needs some work in the drivers-x86 tree
On 07-Feb-19 9:25 PM, Andy Shevchenko wrote: On Thu, Feb 7, 2019 at 4:06 AM Bhardwaj, Rajneesh wrote: On 07-Feb-19 4:27 AM, Stephen Rothwell wrote: Hi all, In commit 4284dc008f43 ("platform/x86: intel_pmc_core: Fix file permissions for ltr_show") Fixes tag Fixes: 63cde0c16c67 ("platform/x86: intel_pmc_core: Show Latency Tolerance info") has these problem(s): - Target SHA1 does not exist Did you mean: 2eb150558bb7 ("platform/x86: intel_pmc_core: Show Latency Tolerance info") Yes, upstream commit is 2eb150558bb79ee01c39b64c2868216c0be2904f. For some reason when i do git show on my repo with both these SHA1 i see the same patch. I will fix this in next version. Hmm... this came to our published branch, i.e. for-next, would it be better to update it via rebasing? Darren, what do you think? Hi Andy, I have corrected this in v2 anyway and i sent to upstream today, just in case you prefer it over rebasing. https://patchwork.kernel.org/patch/10810123/
Re: [PATCH 09/10] platform/x86: intel_pmc_core: Add Package cstates residency info
On 12-Feb-19 3:55 PM, Andy Shevchenko wrote: On Mon, Feb 11, 2019 at 8:32 PM Bhardwaj, Rajneesh wrote: On 11-Feb-19 10:11 PM, Anshuman Gupta wrote: On Fri, Feb 01, 2019 at 01:02:33PM +0530, Rajneesh Bhardwaj wrote: This patch introduces a new debugfs entry to read current Package cstate residency counters. A similar variant of this patch was discussed earlier "https://patchwork.kernel.org/patch/9908563/; but didn't make it into mainline for various reasons. Current version only adds debugfs entry which is quite useful for S0ix debug but excludes the exported API that was there in initial version. Though there are tools like turbostat and socwatch which can also show this info but sometimes its more practical to have it here as it's hard to switch between various tools for S0ix debug when pmc_core driver is the primary debug tool. Internal and external customers have requested for this patch to be included in the PMC driver on many occasions and Google Chrome OS team has already included it in their builds. This becomes handy when requesting logs from external customers who may not always have above mentioned tools in their integrated kernel builds. Tested-by: Anshuman Gupta Acked-by: Anshuman Gupta Hi Andy, Darren - Can i used Acked-and-tested-by instead of these two? Better to split. I didn't see in common practice acked-and-tested. Rather reviewed-and-tested, or reported-and-tested. Sure Andy, I can do as you suggest but i am confused now because on another patch i've got "Acked-and-tested-by" only i.e. just one single tag. So for consistency, what should i do? I also noticed that this tag is used about 59 times in Linux kernel and i even found one "Reported-Acked-and-Tested-by" too :) though i understand these may not be acceptable by all Maintainers. So, please guide.
Re: [PATCH 09/10] platform/x86: intel_pmc_core: Add Package cstates residency info
On 11-Feb-19 10:11 PM, Anshuman Gupta wrote: On Fri, Feb 01, 2019 at 01:02:33PM +0530, Rajneesh Bhardwaj wrote: This patch introduces a new debugfs entry to read current Package cstate residency counters. A similar variant of this patch was discussed earlier "https://patchwork.kernel.org/patch/9908563/; but didn't make it into mainline for various reasons. Current version only adds debugfs entry which is quite useful for S0ix debug but excludes the exported API that was there in initial version. Though there are tools like turbostat and socwatch which can also show this info but sometimes its more practical to have it here as it's hard to switch between various tools for S0ix debug when pmc_core driver is the primary debug tool. Internal and external customers have requested for this patch to be included in the PMC driver on many occasions and Google Chrome OS team has already included it in their builds. This becomes handy when requesting logs from external customers who may not always have above mentioned tools in their integrated kernel builds. Package cstate residency MSRs provide useful debug information about system idle states. In idle states system must enter deeper Package cstates. Package cstates depend not only on Core cstates but also on various IP block's power gating status and LTR values. For Intel Core SoCs Package C10 entry is a must for deeper sleep states such as S0ix. "Suspend-to-idle" should ideally take this path: PC0 -> PC10 -> S0ix. For S0ix debug, its logical to check for Package C10 residency first if for some reason system fails to enter S0ix. Please refer to this link for MSR details: https://software.intel.com/sites/default/files/managed/22/0d/335592-sdm-vol-4.pdf Usage: cat /sys/kernel/debug/pmc_core/package_cstate_show Package C2 : 0xec2e21735f Package C3 : 0xc30113ba4 Package C6 : 0x9ef4be15c5 Package C7 : 0x1e011904 Package C8 : 0x3c5653cfe5a Package C9 : 0x0 Package C10 : 0x16fff4289 Cc: Arjan van de Ven Cc: "David E. Box" Cc: Srinivas Pandruvada Cc: Anshuman Gupta Cc: Len Brown Cc: Rafael J. Wysocki Signed-off-by: Rajneesh Bhardwaj Tested-by: Anshuman Gupta Acked-by: Anshuman Gupta Thanks Anshuman. Hi Andy, Darren - Can i used Acked-and-tested-by instead of these two? --- drivers/platform/x86/intel_pmc_core.c | 38 +++ drivers/platform/x86/intel_pmc_core.h | 1 + 2 files changed, 39 insertions(+) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 400946b7a3b5..4e7aa1711148 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -22,11 +22,24 @@ #include #include +#include #include "intel_pmc_core.h" static struct pmc_dev pmc; +/* PKGC MSRs are common across Intel Core SoCs */ +static const struct pmc_bit_map msr_map[] = { + {"Package C2", MSR_PKG_C2_RESIDENCY}, + {"Package C3", MSR_PKG_C3_RESIDENCY}, + {"Package C6", MSR_PKG_C6_RESIDENCY}, + {"Package C7", MSR_PKG_C7_RESIDENCY}, + {"Package C8", MSR_PKG_C8_RESIDENCY}, + {"Package C9", MSR_PKG_C9_RESIDENCY}, + {"Package C10", MSR_PKG_C10_RESIDENCY}, + {} +}; + static const struct pmc_bit_map spt_pll_map[] = { {"MIPI PLL", SPT_PMC_BIT_MPHY_CMN_LANE0}, {"GEN2 USB2PCIE2 PLL",SPT_PMC_BIT_MPHY_CMN_LANE1}, @@ -129,6 +142,7 @@ static const struct pmc_reg_map spt_reg_map = { .mphy_sts = spt_mphy_map, .pll_sts = spt_pll_map, .ltr_show_sts = spt_ltr_show_map, + .msr_sts = msr_map, .slp_s0_offset = SPT_PMC_SLP_S0_RES_COUNTER_OFFSET, .ltr_ignore_offset = SPT_PMC_LTR_IGNORE_OFFSET, .regmap_length = SPT_PMC_MMIO_REG_LEN, @@ -318,6 +332,7 @@ static const struct pmc_reg_map cnp_reg_map = { .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET, .slps0_dbg_maps = cnp_slps0_dbg_maps, .ltr_show_sts = cnp_ltr_show_map, + .msr_sts = msr_map, .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET, .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET, .regmap_length = CNP_PMC_MMIO_REG_LEN, @@ -333,6 +348,7 @@ static const struct pmc_reg_map icl_reg_map = { .slp_s0_offset = CNP_PMC_SLP_S0_RES_COUNTER_OFFSET, .slps0_dbg_maps = cnp_slps0_dbg_maps, .ltr_show_sts = cnp_ltr_show_map, + .msr_sts = msr_map, .slps0_dbg_offset = CNP_PMC_SLPS0_DBG_OFFSET, .ltr_ignore_offset = CNP_PMC_LTR_IGNORE_OFFSET, .regmap_length = CNP_PMC_MMIO_REG_LEN, @@ -709,6 +725,25 @@ static int pmc_core_ltr_show(struct seq_file *s, void *unused) } DEFINE_SHOW_ATTRIBUTE(pmc_core_ltr); +static int pmc_core_pkgc_show(struct seq_file *s, void *unused) +{ + struct pmc_dev *pmcdev = s->private; +
Re: [PATCH 00/10] ICL support and other enhancements for PMC Core
On 05-Feb-19 11:36 PM, Andy Shevchenko wrote: On Fri, Feb 1, 2019 at 9:32 AM Rajneesh Bhardwaj wrote: This patch series provides Icelake support for PMC Core driver and while doing so it introduces the Icelake Mobile to intel-family.h as per the CPUID from below Coreboot link https://github.com/coreboot/coreboot/blob/5ebcea33cd358bc5bccaa156b13a6ef25df6/src/soc/intel/common/block/include/intelblocks/mp_init.h and provides some fixes and enhancements to the driver. I have applied first 5 patches (fixes) to my review and testing queue, thanks! Thank you Andy :) I'll wait for the next version next week, based on what would be promoted to our for-next branch and addressed comments from reviewers. Fine, I'll look forward to submit remaining patches after addressing all received feedback thus far, next week. This series: -adds ICL U/Y CPUID to intel-family.h -enable PMC driver for ICL. -introduce a new "package cstate show" feature -fixes a customer issue related to S0ix on latest HP laptops. -fixes some minor bugs Rajneesh Bhardwaj (10): platform/x86: intel_pmc_core: Handle CFL regmap properly platform/x86: intel_pmc_core: Fix PCH IP sts reading platform/x86: intel_pmc_core: Fix PCH IP name platform/x86: intel_pmc_core: Fix file permissions for ltr_show platform/x86: intel_pmc_core: Include Reserved IP for LTR x86/cpu: Add Icelake to Intel family platform/x86: intel_pmc_core: Convert to INTEL_CPU_FAM6 macro platform/x86: intel_pmc_core: Add ICL platform support platform/x86: intel_pmc_core: Add Package cstates residency info platform/x86: intel_pmc_core: Quirk to ignore XTAL shutdown arch/x86/include/asm/intel-family.h | 2 + drivers/platform/x86/intel_pmc_core.c | 155 +- drivers/platform/x86/intel_pmc_core.h | 14 ++- 3 files changed, 145 insertions(+), 26 deletions(-) -- 2.17.1
Re: [PATCH 06/10] x86/cpu: Add Icelake to Intel family
On 04-Feb-19 11:04 PM, Thomas Gleixner wrote: On Fri, 1 Feb 2019, Rajneesh Bhardwaj wrote: Add CPUID of Icelake (ICL) mobile processors to Intel family list. The Information related to ICL CPUID is referenced from below Coreboot project link. https://github.com/coreboot/coreboot/blob/5ebcea33cd358bc5bccaa156b13a6ef25df6/src/soc/intel/common/block/include/intelblocks/mp_init.h Coreboot files are hardly authoritive information about CPUIDs. It may take a while for it to be published in the SDMs. diff --git a/arch/x86/include/asm/intel-family.h b/arch/x86/include/asm/intel-family.h index 0dd6b0f4000e..12b65bdb3d80 100644 --- a/arch/x86/include/asm/intel-family.h +++ b/arch/x86/include/asm/intel-family.h @@ -52,6 +52,8 @@ #define INTEL_FAM6_CANNONLAKE_MOBILE 0x66 +#define INTEL_FAM6_ICELAKE_MOBILE 0x7E Please use TABs instead of a random number of spaces. Sure, I will fix this. Thanks, tglx
Re: [PATCH v3 3/3] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
On 03-Nov-18 12:02 AM, Andy Shevchenko wrote: On Fri, Nov 2, 2018 at 12:37 PM Rajneesh Bhardwaj wrote: The LTR values follow PCIE LTR encoding format and can be decoded as per https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf This adds support to translate the raw LTR values as read from the PMC to meaningful values in nanosecond units of time. +#include I told you something different, i.e. put this header where you _use_ it, i.o.w into the header file. Oops! Will move it to the header. +#define LTR_REQ_NONSNOOP BIT(31) +#define LTR_REQ_SNOOP BIT(15) +#define LTR_DECODED_VALGENMASK(9, 0) +#define LTR_DECODED_SCALE GENMASK(12, 10) If these are in one register, please keep ordered by start bit. Sure, will do. The rest is fine. Many thanks again for your detailed review.
Re: [PATCH v3 3/3] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
On 03-Nov-18 12:02 AM, Andy Shevchenko wrote: On Fri, Nov 2, 2018 at 12:37 PM Rajneesh Bhardwaj wrote: The LTR values follow PCIE LTR encoding format and can be decoded as per https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf This adds support to translate the raw LTR values as read from the PMC to meaningful values in nanosecond units of time. +#include I told you something different, i.e. put this header where you _use_ it, i.o.w into the header file. Oops! Will move it to the header. +#define LTR_REQ_NONSNOOP BIT(31) +#define LTR_REQ_SNOOP BIT(15) +#define LTR_DECODED_VALGENMASK(9, 0) +#define LTR_DECODED_SCALE GENMASK(12, 10) If these are in one register, please keep ordered by start bit. Sure, will do. The rest is fine. Many thanks again for your detailed review.
Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Show Latency Tolerance info
Thanks again for your time. My response inline. On 02-Nov-18 11:57 PM, Andy Shevchenko wrote: On Fri, Nov 2, 2018 at 12:29 PM Rajneesh Bhardwaj wrote: This adds support to show the Latency Tolerance Reporting for the IPs on the PCH as reported by the PMC. The format shown here is raw LTR data payload that can further be decoded as per the PCI specification. This also fixes some minor alignment issues in the header file by removing spaces and converting to tabs at some places. Thanks for the update, my comments below. Signed-off-by: Rajneesh Bhardwaj [andy: fixed output to avoid LTR duplication and put space after colon] Signed-off-by: Andy Shevchenko You incorporated changes I proposed — good! But please, don't do my job with signing stuff, etc. Just mention what you did in the changelog. Okay. I downloaded the patch applied on review-andy branch and worked on it. Those two lines came along with the downloaded patch but i understood your concern now. +static const struct pmc_bit_map spt_ltr_show_map[] = { + {"SOUTHPORT_A", SPT_PMC_LTR_SPA}, + {"SOUTHPORT_B", SPT_PMC_LTR_SPB}, + {"SATA",SPT_PMC_LTR_SATA}, + {"GIGABIT_ETHERNET",SPT_PMC_LTR_GBE}, + {"XHCI",SPT_PMC_LTR_XHCI}, + /* IP 5 is reserved */ Since we dropped explicit numbering, this line and similar sounds redundant. Fine. + {"ME", SPT_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"EVA", SPT_PMC_LTR_EVA}, + {"SOUTHPORT_C", SPT_PMC_LTR_SPC}, + {"HD_AUDIO",SPT_PMC_LTR_AZ}, + /* IP 10 is reserved */ + {"LPSS",SPT_PMC_LTR_LPSS}, + {"SOUTHPORT_D", SPT_PMC_LTR_SPD}, + {"SOUTHPORT_E", SPT_PMC_LTR_SPE}, + {"CAMERA", SPT_PMC_LTR_CAM}, + {"ESPI",SPT_PMC_LTR_ESPI}, + {"SCC", SPT_PMC_LTR_SCC}, + {"ISH", SPT_PMC_LTR_ISH}, + /* Below two cannot be used for LTR_IGNORE */ + {"CURRENT_PLATFORM",SPT_PMC_LTR_CUR_PLT}, + {"AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT}, + {} +}; /* Cannonlake Power Management Controller register offsets */ -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C -#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C -#define CNP_PMC_PM_CFG_OFFSET 0x1818 +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C +#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C +#define CNP_PMC_PM_CFG_OFFSET 0x1818 #define CNP_PMC_SLPS0_DBG_OFFSET 0x10B4 Can we preserve ordering? Doesn't harm, will reorder based on offsets increasing order. /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */ -#define CNP_PMC_HOST_PPFEAR0A 0x1D90 +#define CNP_PMC_HOST_PPFEAR0A 0x1D90 What's wrong with this line? Why is it changed? This and below are edited to convert spaces to tabs. This is captured in commit message too. -#define CNP_PMC_MMIO_REG_LEN 0x2000 -#define CNP_PPFEAR_NUM_ENTRIES 8 -#define CNP_PMC_READ_DISABLE_BIT 22 +#define CNP_PMC_MMIO_REG_LEN 0x2000 +#define CNP_PPFEAR_NUM_ENTRIES 8 +#define CNP_PMC_READ_DISABLE_BIT 22 What happened to these lines? same as above. #define CNP_PMC_LATCH_SLPS0_EVENTS BIT(31) Perhaps + blank line here This is from a previous commit but i think its better to move it up the block and insert one blank line after. If we insert one blank line after its current position, it looks odd and cuts the logical block unnecessarily. +#define CNP_PMC_LTR_CUR_PLT0x1B50 +#define CNP_PMC_LTR_CUR_ASLT 0x1B54 +#define CNP_PMC_LTR_SPA0x1B60 +#define CNP_PMC_LTR_SPB0x1B64 +#define CNP_PMC_LTR_SATA 0x1B68 +#define CNP_PMC_LTR_GBE0x1B6C +#define CNP_PMC_LTR_XHCI 0x1B70 +#define CNP_PMC_LTR_ME 0x1B78 +#define CNP_PMC_LTR_EVA0x1B7C +#define CNP_PMC_LTR_SPC0x1B80 +#define CNP_PMC_LTR_AZ 0x1B84 +#define CNP_PMC_LTR_LPSS 0x1B8C +#define CNP_PMC_LTR_CAM0x1B90 +#define CNP_PMC_LTR_SPD0x1B94 +#define CNP_PMC_LTR_SPE0x1B98 +#define CNP_PMC_LTR_ESPI 0x1B9C +#define CNP_PMC_LTR_SCC0x1BA0 +#define CNP_PMC_LTR_ISH0x1BA4 +#define CNP_PMC_LTR_CNV0x1BF0 +#define CNP_PMC_LTR_EMMC 0x1BF4 +#define
Re: [PATCH v3 1/3] platform/x86: intel_pmc_core: Show Latency Tolerance info
Thanks again for your time. My response inline. On 02-Nov-18 11:57 PM, Andy Shevchenko wrote: On Fri, Nov 2, 2018 at 12:29 PM Rajneesh Bhardwaj wrote: This adds support to show the Latency Tolerance Reporting for the IPs on the PCH as reported by the PMC. The format shown here is raw LTR data payload that can further be decoded as per the PCI specification. This also fixes some minor alignment issues in the header file by removing spaces and converting to tabs at some places. Thanks for the update, my comments below. Signed-off-by: Rajneesh Bhardwaj [andy: fixed output to avoid LTR duplication and put space after colon] Signed-off-by: Andy Shevchenko You incorporated changes I proposed — good! But please, don't do my job with signing stuff, etc. Just mention what you did in the changelog. Okay. I downloaded the patch applied on review-andy branch and worked on it. Those two lines came along with the downloaded patch but i understood your concern now. +static const struct pmc_bit_map spt_ltr_show_map[] = { + {"SOUTHPORT_A", SPT_PMC_LTR_SPA}, + {"SOUTHPORT_B", SPT_PMC_LTR_SPB}, + {"SATA",SPT_PMC_LTR_SATA}, + {"GIGABIT_ETHERNET",SPT_PMC_LTR_GBE}, + {"XHCI",SPT_PMC_LTR_XHCI}, + /* IP 5 is reserved */ Since we dropped explicit numbering, this line and similar sounds redundant. Fine. + {"ME", SPT_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"EVA", SPT_PMC_LTR_EVA}, + {"SOUTHPORT_C", SPT_PMC_LTR_SPC}, + {"HD_AUDIO",SPT_PMC_LTR_AZ}, + /* IP 10 is reserved */ + {"LPSS",SPT_PMC_LTR_LPSS}, + {"SOUTHPORT_D", SPT_PMC_LTR_SPD}, + {"SOUTHPORT_E", SPT_PMC_LTR_SPE}, + {"CAMERA", SPT_PMC_LTR_CAM}, + {"ESPI",SPT_PMC_LTR_ESPI}, + {"SCC", SPT_PMC_LTR_SCC}, + {"ISH", SPT_PMC_LTR_ISH}, + /* Below two cannot be used for LTR_IGNORE */ + {"CURRENT_PLATFORM",SPT_PMC_LTR_CUR_PLT}, + {"AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT}, + {} +}; /* Cannonlake Power Management Controller register offsets */ -#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C -#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C -#define CNP_PMC_PM_CFG_OFFSET 0x1818 +#define CNP_PMC_SLP_S0_RES_COUNTER_OFFSET 0x193C +#define CNP_PMC_LTR_IGNORE_OFFSET 0x1B0C +#define CNP_PMC_PM_CFG_OFFSET 0x1818 #define CNP_PMC_SLPS0_DBG_OFFSET 0x10B4 Can we preserve ordering? Doesn't harm, will reorder based on offsets increasing order. /* Cannonlake: PGD PFET Enable Ack Status Register(s) start */ -#define CNP_PMC_HOST_PPFEAR0A 0x1D90 +#define CNP_PMC_HOST_PPFEAR0A 0x1D90 What's wrong with this line? Why is it changed? This and below are edited to convert spaces to tabs. This is captured in commit message too. -#define CNP_PMC_MMIO_REG_LEN 0x2000 -#define CNP_PPFEAR_NUM_ENTRIES 8 -#define CNP_PMC_READ_DISABLE_BIT 22 +#define CNP_PMC_MMIO_REG_LEN 0x2000 +#define CNP_PPFEAR_NUM_ENTRIES 8 +#define CNP_PMC_READ_DISABLE_BIT 22 What happened to these lines? same as above. #define CNP_PMC_LATCH_SLPS0_EVENTS BIT(31) Perhaps + blank line here This is from a previous commit but i think its better to move it up the block and insert one blank line after. If we insert one blank line after its current position, it looks odd and cuts the logical block unnecessarily. +#define CNP_PMC_LTR_CUR_PLT0x1B50 +#define CNP_PMC_LTR_CUR_ASLT 0x1B54 +#define CNP_PMC_LTR_SPA0x1B60 +#define CNP_PMC_LTR_SPB0x1B64 +#define CNP_PMC_LTR_SATA 0x1B68 +#define CNP_PMC_LTR_GBE0x1B6C +#define CNP_PMC_LTR_XHCI 0x1B70 +#define CNP_PMC_LTR_ME 0x1B78 +#define CNP_PMC_LTR_EVA0x1B7C +#define CNP_PMC_LTR_SPC0x1B80 +#define CNP_PMC_LTR_AZ 0x1B84 +#define CNP_PMC_LTR_LPSS 0x1B8C +#define CNP_PMC_LTR_CAM0x1B90 +#define CNP_PMC_LTR_SPD0x1B94 +#define CNP_PMC_LTR_SPE0x1B98 +#define CNP_PMC_LTR_ESPI 0x1B9C +#define CNP_PMC_LTR_SCC0x1BA0 +#define CNP_PMC_LTR_ISH0x1BA4 +#define CNP_PMC_LTR_CNV0x1BF0 +#define CNP_PMC_LTR_EMMC 0x1BF4 +#define
Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
Hi Andy, Thanks for your review. My comments below. If you agree then i can quickly send v3 addressing all suggestions so we can make it in time for 4.20 merge window. On 19-Oct-18 6:04 PM, Andy Shevchenko wrote: On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj wrote: The LTR values follow PCIE LTR encoding format and can be decoded as per https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf This adds support to translate the raw LTR values as read from the PMC to meaningful values in nanosecond units of time. While I have pushed this to my review and testing queue, it needs a bit more work. See my comments below. +static u32 convert_ltr_scale(u32 val) +{ + u32 scale = 0; Redundant, see below. + /* +* As per PCIE specification supporting document +* ECN_LatencyTolnReporting_14Aug08.pdf the Latency +* Tolerance Reporting data payload is encoded in a +* 3 bit scale and 10 bit value fields. Values are +* multiplied by the indicated scale to yield an absolute time +* value, expressible in a range from 1 nanosecond to +* 2^25*(2^10-1) = 34,326,183,936 nanoseconds. +* +* scale encoding is as follows: +* +* -- +* |scale factor| Multiplier (ns) | +* -- +* |0 | 1 | +* |1 | 32 | +* |2 | 1024| +* |3 | 32768 | +* |4 | 1048576 | +* |5 | 33554432| +* |6 | Invalid | +* |7 | Invalid | +* -- +*/ + if (val > 5) + pr_warn("Invalid LTR scale factor.\n"); if (...) { pr_warn(...); // Btw, Does it recoverable state? What user will get with returned 0 as a multiplier? return 0; // Btw, is 0 fits better than ~0? How hw would behave with this value? } We show 0 LTR for invalid scale as PMC output is junk sometimes. + else + scale = 1U << (5 * (val)); + + return scale; return 1U << (5 * val); We intend to return 0 so for invalid LTR scale. This will make retuen non zero and we dont want that. +} for (index = 0; map[index].name ; index++) { - seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index, - map[index].name, - pmc_core_reg_read(pmcdev, map[index].bit_mask)); We use 32 characters for the names. Here are two minor issues: - inconsistency with the rest intentional. - ping-pong style of programming (you changed 32 to 24 in the same series where you introduced 32 in the first place). This is because the formatted output looks better with this width. I used 32 for the earlier patch because its consistent with rest and also does not look bad on screen. + decoded_snoop_ltr = decoded_non_snoop_ltr = 0; + ltr_raw_data = pmc_core_reg_read(pmcdev, +map[index].bit_mask); + snoop_ltr = ltr_raw_data & ~MTPMC_MASK; + nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK; + + if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) { + scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr); + val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr); + decoded_non_snoop_ltr = val * convert_ltr_scale(scale); + } + + if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) { + scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr); + val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr); + decoded_snoop_ltr = val * convert_ltr_scale(scale); + } + + seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n", Here 0x%-16x would look a bit strange and difficult to parse. 0x%016x much better. Sure, I can test how it looks with 0x%016x and modify it. After you remove the index, it would give you 4 more characters, though it 4 less than 8 you got from reducing 32 to 24. I plan to keep the index as is. Reason for this is mentioned in previous patch that introduces this index. OTOH, those long texts perhaps may be compressed somehow, at least remove LTR duplicating from the last two. Remove spaces after '\t' as well. Noted. + index, map[index].name, ltr_raw_data, + decoded_non_snoop_ltr, + decoded_snoop_ltr); } return 0; } ---
Re: [PATCH v2 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
Hi Andy, Thanks for your review. My comments below. If you agree then i can quickly send v3 addressing all suggestions so we can make it in time for 4.20 merge window. On 19-Oct-18 6:04 PM, Andy Shevchenko wrote: On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj wrote: The LTR values follow PCIE LTR encoding format and can be decoded as per https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf This adds support to translate the raw LTR values as read from the PMC to meaningful values in nanosecond units of time. While I have pushed this to my review and testing queue, it needs a bit more work. See my comments below. +static u32 convert_ltr_scale(u32 val) +{ + u32 scale = 0; Redundant, see below. + /* +* As per PCIE specification supporting document +* ECN_LatencyTolnReporting_14Aug08.pdf the Latency +* Tolerance Reporting data payload is encoded in a +* 3 bit scale and 10 bit value fields. Values are +* multiplied by the indicated scale to yield an absolute time +* value, expressible in a range from 1 nanosecond to +* 2^25*(2^10-1) = 34,326,183,936 nanoseconds. +* +* scale encoding is as follows: +* +* -- +* |scale factor| Multiplier (ns) | +* -- +* |0 | 1 | +* |1 | 32 | +* |2 | 1024| +* |3 | 32768 | +* |4 | 1048576 | +* |5 | 33554432| +* |6 | Invalid | +* |7 | Invalid | +* -- +*/ + if (val > 5) + pr_warn("Invalid LTR scale factor.\n"); if (...) { pr_warn(...); // Btw, Does it recoverable state? What user will get with returned 0 as a multiplier? return 0; // Btw, is 0 fits better than ~0? How hw would behave with this value? } We show 0 LTR for invalid scale as PMC output is junk sometimes. + else + scale = 1U << (5 * (val)); + + return scale; return 1U << (5 * val); We intend to return 0 so for invalid LTR scale. This will make retuen non zero and we dont want that. +} for (index = 0; map[index].name ; index++) { - seq_printf(s, "IP %-2d :%-32s\tRAW LTR: 0x%x\n", index, - map[index].name, - pmc_core_reg_read(pmcdev, map[index].bit_mask)); We use 32 characters for the names. Here are two minor issues: - inconsistency with the rest intentional. - ping-pong style of programming (you changed 32 to 24 in the same series where you introduced 32 in the first place). This is because the formatted output looks better with this width. I used 32 for the earlier patch because its consistent with rest and also does not look bad on screen. + decoded_snoop_ltr = decoded_non_snoop_ltr = 0; + ltr_raw_data = pmc_core_reg_read(pmcdev, +map[index].bit_mask); + snoop_ltr = ltr_raw_data & ~MTPMC_MASK; + nonsnoop_ltr = (ltr_raw_data >> 0x10) & ~MTPMC_MASK; + + if (FIELD_GET(LTR_REQ_NONSNOOP, ltr_raw_data)) { + scale = FIELD_GET(LTR_DECODED_SCALE, nonsnoop_ltr); + val = FIELD_GET(LTR_DECODED_VAL, nonsnoop_ltr); + decoded_non_snoop_ltr = val * convert_ltr_scale(scale); + } + + if (FIELD_GET(LTR_REQ_SNOOP, ltr_raw_data)) { + scale = FIELD_GET(LTR_DECODED_SCALE, snoop_ltr); + val = FIELD_GET(LTR_DECODED_VAL, snoop_ltr); + decoded_snoop_ltr = val * convert_ltr_scale(scale); + } + + seq_printf(s, "IP %-2d :%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n", Here 0x%-16x would look a bit strange and difficult to parse. 0x%016x much better. Sure, I can test how it looks with 0x%016x and modify it. After you remove the index, it would give you 4 more characters, though it 4 less than 8 you got from reducing 32 to 24. I plan to keep the index as is. Reason for this is mentioned in previous patch that introduces this index. OTOH, those long texts perhaps may be compressed somehow, at least remove LTR duplicating from the last two. Remove spaces after '\t' as well. Noted. + index, map[index].name, ltr_raw_data, + decoded_non_snoop_ltr, + decoded_snoop_ltr); } return 0; } ---
Re: [PATCH v2 4/4] platform/x86: intel_telemetry: report debugfs failure
On 19-Oct-18 6:09 PM, Andy Shevchenko wrote: On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj wrote: On some Goldmont based systems such as ASRock J3455M the BIOS may not enable the IPC1 device that provides access to the PMC and PUNIT. In such scenarios, the IOSS and PSS resources from the platform device can not be obtained and result in a invalid telemetry_plt_config which is an internal data structure that holds platform config and is maintained by the telemetry platform driver. This is also applicable to the platforms where the BIOS supports IPC1 device under debug configurations but IPC1 is disabled by user or the policy. This change allows user to know the reason for not seeing entries under /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. Pushed to my review and testing queue, thanks! P.S. I appended one more patch against this file, please check if it's okay. Thank you Andy. I will check it when Infradead is online. Cc: Matt Turner Cc: Len Brown Cc: Souvik Kumar Chakravarty Cc: Kuppuswamy Sathyanarayanan Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779 Acked-by: Matt Turner Signed-off-by: Rajneesh Bhardwaj --- Changes in v2: * Removed print and out label both as suggested by Andy. * changed to pr_info. * Other minor style fixes. drivers/platform/x86/intel_telemetry_debugfs.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c index ffd0474b0531..1423fa8710fd 100644 --- a/drivers/platform/x86/intel_telemetry_debugfs.c +++ b/drivers/platform/x86/intel_telemetry_debugfs.c @@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void) debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data; err = telemetry_pltconfig_valid(); - if (err < 0) + if (err < 0) { + pr_info("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n"); return -ENODEV; + } err = telemetry_debugfs_check_evts(); - if (err < 0) + if (err < 0) { + pr_info("telemetry_debugfs_check_evts failed\n"); return -EINVAL; + } register_pm_notifier(_notifier); -- 2.17.1
Re: [PATCH v2 4/4] platform/x86: intel_telemetry: report debugfs failure
On 19-Oct-18 6:09 PM, Andy Shevchenko wrote: On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj wrote: On some Goldmont based systems such as ASRock J3455M the BIOS may not enable the IPC1 device that provides access to the PMC and PUNIT. In such scenarios, the IOSS and PSS resources from the platform device can not be obtained and result in a invalid telemetry_plt_config which is an internal data structure that holds platform config and is maintained by the telemetry platform driver. This is also applicable to the platforms where the BIOS supports IPC1 device under debug configurations but IPC1 is disabled by user or the policy. This change allows user to know the reason for not seeing entries under /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. Pushed to my review and testing queue, thanks! P.S. I appended one more patch against this file, please check if it's okay. Thank you Andy. I will check it when Infradead is online. Cc: Matt Turner Cc: Len Brown Cc: Souvik Kumar Chakravarty Cc: Kuppuswamy Sathyanarayanan Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779 Acked-by: Matt Turner Signed-off-by: Rajneesh Bhardwaj --- Changes in v2: * Removed print and out label both as suggested by Andy. * changed to pr_info. * Other minor style fixes. drivers/platform/x86/intel_telemetry_debugfs.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c index ffd0474b0531..1423fa8710fd 100644 --- a/drivers/platform/x86/intel_telemetry_debugfs.c +++ b/drivers/platform/x86/intel_telemetry_debugfs.c @@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void) debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data; err = telemetry_pltconfig_valid(); - if (err < 0) + if (err < 0) { + pr_info("Invalid pltconfig, ensure IPC1 device is enabled in BIOS\n"); return -ENODEV; + } err = telemetry_debugfs_check_evts(); - if (err < 0) + if (err < 0) { + pr_info("telemetry_debugfs_check_evts failed\n"); return -EINVAL; + } register_pm_notifier(_notifier); -- 2.17.1
Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info
+ Srinivas On 19-Oct-18 5:42 PM, Andy Shevchenko wrote: On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj wrote: This adds support to show the Latency Tolerance Reporting for the IPs on the PCH as reported by the PMC. The format shown here is raw LTR data payload that can further be decoded as per the PCI specification. This also fixes some minor alignment issues in the header file by removing spaces and converting to tabs at some places. I have pushed slightly modified variant of this patch to my review and testing queue. Though this series needs a bit more work. Thanks again for your review Andy. I see that the infradead server is down at the moment so i haven't seen your modifications yet. http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-andy says gateway timeout. I see inconsistency now with the output with the rest of nodes there. index printing is not needed for those nodes hence i never added. I don't care much, though some can be addressed for no regression. For example, index printing. Please, remove it. I completely forgot about userspace powerful tools. At least two very old and nice can enumerate lines for you. Obviously the index printing is redundant. Index printing is required here (for LTR Show and LTR Ignore) because it paves an obvious and easy way for the users of this driver to know the IP number to be used for LTR ignore. This was specifically requested by some customer and Srinivas asked me to implement this so adding him for his inputs. I am also planning to add some documentation for this driver later so please consider this in current form. Signed-off-by: Rajneesh Bhardwaj --- Changes in v2: * Removed IP # from map and displaying IP # while printing. * Other style fixes as per Andy's suggestion. drivers/platform/x86/intel_pmc_core.c | 73 +++ drivers/platform/x86/intel_pmc_core.h | 56 +--- 2 files changed, 122 insertions(+), 7 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 2d272a3e0176..217a822a8da1 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -110,10 +110,37 @@ static const struct pmc_bit_map spt_pfear_map[] = { {}, }; +static const struct pmc_bit_map spt_ltr_show_map[] = { + {"LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA}, + {"LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB}, + {"LTR_SATA",SPT_PMC_LTR_SATA}, + {"LTR_GIGABIT_ETHERNET",SPT_PMC_LTR_GBE}, + {"LTR_XHCI",SPT_PMC_LTR_XHCI}, + /* IP 5 is reserved */ + {"LTR_ME", SPT_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"LTR_EVA", SPT_PMC_LTR_EVA}, + {"LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC}, + {"LTR_HD_AUDIO",SPT_PMC_LTR_AZ}, + /* IP 10 is reserved */ + {"LTR_LPSS",SPT_PMC_LTR_LPSS}, + {"LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD}, + {"LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE}, + {"LTR_CAMERA", SPT_PMC_LTR_CAM}, + {"LTR_ESPI",SPT_PMC_LTR_ESPI}, + {"LTR_SCC", SPT_PMC_LTR_SCC}, + {"LTR_ISH", SPT_PMC_LTR_ISH}, + /* Below two cannot be used for LTR_IGNORE */ + {"LTR_CURRENT_PLATFORM",SPT_PMC_LTR_CUR_PLT}, + {"LTR_AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT}, + {} +}; + static const struct pmc_reg_map spt_reg_map = { .pfear_sts = spt_pfear_map, .mphy_sts = spt_mphy_map, .pll_sts = spt_pll_map, + .ltr_show_sts = spt_ltr_show_map, .slp_s0_offset = SPT_PMC_SLP_S0_RES_COUNTER_OFFSET, .ltr_ignore_offset = SPT_PMC_LTR_IGNORE_OFFSET, .regmap_length = SPT_PMC_MMIO_REG_LEN, @@ -252,10 +279,39 @@ static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = { NULL, }; +static const struct pmc_bit_map cnp_ltr_show_map[] = { + {"LTR_SOUTHPORT_A", CNP_PMC_LTR_SPA}, + {"LTR_SOUTHPORT_B", CNP_PMC_LTR_SPB}, + {"LTR_SATA",CNP_PMC_LTR_SATA}, + {"LTR_GIGABIT_ETHERNET",CNP_PMC_LTR_GBE}, + {"LTR_XHCI",CNP_PMC_LTR_XHCI}, + /* IP 5 is reserved */ + {"LTR_ME", CNP_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"LTR_EVA", CNP_PMC_LTR_EVA}, + {"LTR_SOUTHPORT_C", CNP_PMC_LTR_SPC}, + {"LTR_HD_AUDIO",CNP_PMC_LTR_AZ}, + {"LTR_CNV", CNP_PMC_LTR_CNV}, + {"LTR_LPSS",CNP_PMC_LTR_LPSS}, + {"LTR_SOUTHPORT_D", CNP_PMC_LTR_SPD}, + {"LTR_SOUTHPORT_E",
Re: [PATCH v2 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info
+ Srinivas On 19-Oct-18 5:42 PM, Andy Shevchenko wrote: On Sat, Oct 6, 2018 at 9:54 AM Rajneesh Bhardwaj wrote: This adds support to show the Latency Tolerance Reporting for the IPs on the PCH as reported by the PMC. The format shown here is raw LTR data payload that can further be decoded as per the PCI specification. This also fixes some minor alignment issues in the header file by removing spaces and converting to tabs at some places. I have pushed slightly modified variant of this patch to my review and testing queue. Though this series needs a bit more work. Thanks again for your review Andy. I see that the infradead server is down at the moment so i haven't seen your modifications yet. http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/review-andy says gateway timeout. I see inconsistency now with the output with the rest of nodes there. index printing is not needed for those nodes hence i never added. I don't care much, though some can be addressed for no regression. For example, index printing. Please, remove it. I completely forgot about userspace powerful tools. At least two very old and nice can enumerate lines for you. Obviously the index printing is redundant. Index printing is required here (for LTR Show and LTR Ignore) because it paves an obvious and easy way for the users of this driver to know the IP number to be used for LTR ignore. This was specifically requested by some customer and Srinivas asked me to implement this so adding him for his inputs. I am also planning to add some documentation for this driver later so please consider this in current form. Signed-off-by: Rajneesh Bhardwaj --- Changes in v2: * Removed IP # from map and displaying IP # while printing. * Other style fixes as per Andy's suggestion. drivers/platform/x86/intel_pmc_core.c | 73 +++ drivers/platform/x86/intel_pmc_core.h | 56 +--- 2 files changed, 122 insertions(+), 7 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 2d272a3e0176..217a822a8da1 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -110,10 +110,37 @@ static const struct pmc_bit_map spt_pfear_map[] = { {}, }; +static const struct pmc_bit_map spt_ltr_show_map[] = { + {"LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA}, + {"LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB}, + {"LTR_SATA",SPT_PMC_LTR_SATA}, + {"LTR_GIGABIT_ETHERNET",SPT_PMC_LTR_GBE}, + {"LTR_XHCI",SPT_PMC_LTR_XHCI}, + /* IP 5 is reserved */ + {"LTR_ME", SPT_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"LTR_EVA", SPT_PMC_LTR_EVA}, + {"LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC}, + {"LTR_HD_AUDIO",SPT_PMC_LTR_AZ}, + /* IP 10 is reserved */ + {"LTR_LPSS",SPT_PMC_LTR_LPSS}, + {"LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD}, + {"LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE}, + {"LTR_CAMERA", SPT_PMC_LTR_CAM}, + {"LTR_ESPI",SPT_PMC_LTR_ESPI}, + {"LTR_SCC", SPT_PMC_LTR_SCC}, + {"LTR_ISH", SPT_PMC_LTR_ISH}, + /* Below two cannot be used for LTR_IGNORE */ + {"LTR_CURRENT_PLATFORM",SPT_PMC_LTR_CUR_PLT}, + {"LTR_AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT}, + {} +}; + static const struct pmc_reg_map spt_reg_map = { .pfear_sts = spt_pfear_map, .mphy_sts = spt_mphy_map, .pll_sts = spt_pll_map, + .ltr_show_sts = spt_ltr_show_map, .slp_s0_offset = SPT_PMC_SLP_S0_RES_COUNTER_OFFSET, .ltr_ignore_offset = SPT_PMC_LTR_IGNORE_OFFSET, .regmap_length = SPT_PMC_MMIO_REG_LEN, @@ -252,10 +279,39 @@ static const struct pmc_bit_map *cnp_slps0_dbg_maps[] = { NULL, }; +static const struct pmc_bit_map cnp_ltr_show_map[] = { + {"LTR_SOUTHPORT_A", CNP_PMC_LTR_SPA}, + {"LTR_SOUTHPORT_B", CNP_PMC_LTR_SPB}, + {"LTR_SATA",CNP_PMC_LTR_SATA}, + {"LTR_GIGABIT_ETHERNET",CNP_PMC_LTR_GBE}, + {"LTR_XHCI",CNP_PMC_LTR_XHCI}, + /* IP 5 is reserved */ + {"LTR_ME", CNP_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"LTR_EVA", CNP_PMC_LTR_EVA}, + {"LTR_SOUTHPORT_C", CNP_PMC_LTR_SPC}, + {"LTR_HD_AUDIO",CNP_PMC_LTR_AZ}, + {"LTR_CNV", CNP_PMC_LTR_CNV}, + {"LTR_LPSS",CNP_PMC_LTR_LPSS}, + {"LTR_SOUTHPORT_D", CNP_PMC_LTR_SPD}, + {"LTR_SOUTHPORT_E",
Re: [PATCH] MAINTAINERS: intel_telemetry: Update maintainers info
On 09-Oct-18 5:07 PM, Andy Shevchenko wrote: On Mon, Oct 8, 2018 at 5:44 PM Rajneesh Bhardwaj wrote: Add myself and David as the new maintainers for Intel Telemetry driver. There is no explanation why. (Like previous one left the company, and / or you are doing set of the drivers related to PMC where telemetry is part of, etc) Hi Andy, Souvik no longer works with Intel. I and David are volunteering to take up the task on his behalf since we do work on drivers which are pretty close to this one in terms of functionalities. Thanks Rajneesh Signed-off-by: Box, David E Signed-off-by: Rajneesh Bhardwaj --- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 22065048d89d..f33ebd1b8034 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7530,7 +7530,8 @@ F:arch/x86/crypto/sha*-mb/ F: crypto/mcryptd.c INTEL TELEMETRY DRIVER -M: Souvik Kumar Chakravarty +M: Rajneesh Bhardwaj +M: Box, David E L: platform-driver-...@vger.kernel.org S: Maintained F: arch/x86/include/asm/intel_telemetry.h -- 2.17.1
Re: [PATCH] MAINTAINERS: intel_telemetry: Update maintainers info
On 09-Oct-18 5:07 PM, Andy Shevchenko wrote: On Mon, Oct 8, 2018 at 5:44 PM Rajneesh Bhardwaj wrote: Add myself and David as the new maintainers for Intel Telemetry driver. There is no explanation why. (Like previous one left the company, and / or you are doing set of the drivers related to PMC where telemetry is part of, etc) Hi Andy, Souvik no longer works with Intel. I and David are volunteering to take up the task on his behalf since we do work on drivers which are pretty close to this one in terms of functionalities. Thanks Rajneesh Signed-off-by: Box, David E Signed-off-by: Rajneesh Bhardwaj --- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 22065048d89d..f33ebd1b8034 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7530,7 +7530,8 @@ F:arch/x86/crypto/sha*-mb/ F: crypto/mcryptd.c INTEL TELEMETRY DRIVER -M: Souvik Kumar Chakravarty +M: Rajneesh Bhardwaj +M: Box, David E L: platform-driver-...@vger.kernel.org S: Maintained F: arch/x86/include/asm/intel_telemetry.h -- 2.17.1
Re: [PATCH 2/4] platform/x86: intel_pmc_core: Fix LTR IGNORE Max offset
Thank you. On 26-Sep-18 7:27 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: Cannonlake PCH allows us to ignore LTR from more IPs than Sunrisepoint PCH so make the LTR ignore platform specific. This looks fine to me. Signed-off-by: Rajneesh Bhardwaj --- drivers/platform/x86/intel_pmc_core.c | 4 +++- drivers/platform/x86/intel_pmc_core.h | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 972735bd4c75..c1330a03523d 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -149,6 +149,7 @@ static const struct pmc_reg_map spt_reg_map = { .ppfear_buckets = SPT_PPFEAR_NUM_ENTRIES, .pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET, .pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT, + .ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED, }; /* Cannonlake: PGD PFET Enable Ack Status Register(s) bitmap */ @@ -320,6 +321,7 @@ static const struct pmc_reg_map cnp_reg_map = { .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES, .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET, .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT, + .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED, }; static inline u8 pmc_core_reg_read_byte(struct pmc_dev *pmcdev, int offset) @@ -566,7 +568,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file, const char __user goto out_unlock; } - if (val > NUM_IP_IGN_ALLOWED) { + if (val > map->ltr_ignore_max) { err = -EINVAL; goto out_unlock; } diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h index b24407048fa1..12663c58f122 100644 --- a/drivers/platform/x86/intel_pmc_core.h +++ b/drivers/platform/x86/intel_pmc_core.h @@ -44,7 +44,7 @@ #define SPT_PMC_READ_DISABLE_BIT 0x16 #define SPT_PMC_MSG_FULL_STS_BIT 0x18 #define NUM_RETRIES100 -#define NUM_IP_IGN_ALLOWED 17 +#define SPT_NUM_IP_IGN_ALLOWED 17 #define SPT_PMC_LTR_CUR_PLT0x350 #define SPT_PMC_LTR_CUR_ASLT 0x354 #define SPT_PMC_LTR_SPA0x360 @@ -153,6 +153,7 @@ enum ppfear_regs { #define CNP_PPFEAR_NUM_ENTRIES 8 #define CNP_PMC_READ_DISABLE_BIT 22 #define CNP_PMC_LATCH_SLPS0_EVENTS BIT(31) +#define CNP_NUM_IP_IGN_ALLOWED 19 #define CNP_PMC_LTR_CUR_PLT0x1B50 #define CNP_PMC_LTR_CUR_ASLT 0x1B54 #define CNP_PMC_LTR_SPA0x1B60 @@ -215,6 +216,7 @@ struct pmc_reg_map { const u32 pm_cfg_offset; const int pm_read_disable_bit; const u32 slps0_dbg_offset; + const u32 ltr_ignore_max; }; /** -- 2.17.1
Re: [PATCH 2/4] platform/x86: intel_pmc_core: Fix LTR IGNORE Max offset
Thank you. On 26-Sep-18 7:27 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: Cannonlake PCH allows us to ignore LTR from more IPs than Sunrisepoint PCH so make the LTR ignore platform specific. This looks fine to me. Signed-off-by: Rajneesh Bhardwaj --- drivers/platform/x86/intel_pmc_core.c | 4 +++- drivers/platform/x86/intel_pmc_core.h | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 972735bd4c75..c1330a03523d 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -149,6 +149,7 @@ static const struct pmc_reg_map spt_reg_map = { .ppfear_buckets = SPT_PPFEAR_NUM_ENTRIES, .pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET, .pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT, + .ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED, }; /* Cannonlake: PGD PFET Enable Ack Status Register(s) bitmap */ @@ -320,6 +321,7 @@ static const struct pmc_reg_map cnp_reg_map = { .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES, .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET, .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT, + .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED, }; static inline u8 pmc_core_reg_read_byte(struct pmc_dev *pmcdev, int offset) @@ -566,7 +568,7 @@ static ssize_t pmc_core_ltr_ignore_write(struct file *file, const char __user goto out_unlock; } - if (val > NUM_IP_IGN_ALLOWED) { + if (val > map->ltr_ignore_max) { err = -EINVAL; goto out_unlock; } diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h index b24407048fa1..12663c58f122 100644 --- a/drivers/platform/x86/intel_pmc_core.h +++ b/drivers/platform/x86/intel_pmc_core.h @@ -44,7 +44,7 @@ #define SPT_PMC_READ_DISABLE_BIT 0x16 #define SPT_PMC_MSG_FULL_STS_BIT 0x18 #define NUM_RETRIES100 -#define NUM_IP_IGN_ALLOWED 17 +#define SPT_NUM_IP_IGN_ALLOWED 17 #define SPT_PMC_LTR_CUR_PLT0x350 #define SPT_PMC_LTR_CUR_ASLT 0x354 #define SPT_PMC_LTR_SPA0x360 @@ -153,6 +153,7 @@ enum ppfear_regs { #define CNP_PPFEAR_NUM_ENTRIES 8 #define CNP_PMC_READ_DISABLE_BIT 22 #define CNP_PMC_LATCH_SLPS0_EVENTS BIT(31) +#define CNP_NUM_IP_IGN_ALLOWED 19 #define CNP_PMC_LTR_CUR_PLT0x1B50 #define CNP_PMC_LTR_CUR_ASLT 0x1B54 #define CNP_PMC_LTR_SPA0x1B60 @@ -215,6 +216,7 @@ struct pmc_reg_map { const u32 pm_cfg_offset; const int pm_read_disable_bit; const u32 slps0_dbg_offset; + const u32 ltr_ignore_max; }; /** -- 2.17.1
Re: [PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On 26-Sep-18 7:26 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: On some Goldmont based systems such as ASRock J3455M the BIOS may not enable the IPC1 device that provides access to the PMC and PUNIT. In such scenarios, the ioss and pss resources from the platform device can IOSS PSS Fine. not be obtained and result in a invalid telemetry_plt_config. What is telemetry_plt_config? Internal data structure that holds platform config, maintained by the telemetry platform driver. This is also applicable to the platforms where the BIOS supports IPC1 device under debug configurations but IPC1 is disabled by user or the policy. This change allows user to know the reason for not seeing entries under /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. Cc: Matt Turner Cc: Len Brown Cc: Souvik Kumar Chakravarty Cc: Kuppuswamy Sathyanarayanan Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779 There should be not a blank line. OK. Acked-by: Matt Turner Signed-off-by: Rajneesh Bhardwaj +exit: + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n"); Completely useless. Device core does it in generic way. If i remove this print then perhaps there is no need of this patch. Reason to print this is that the platform driver / core driver does not show any error. In-fact they are even loaded in module table. OTOH, this debugfs interface fails. This is very confusing to the users if they check the lsmod output so i feel this print might help.
Re: [PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On 26-Sep-18 7:26 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: On some Goldmont based systems such as ASRock J3455M the BIOS may not enable the IPC1 device that provides access to the PMC and PUNIT. In such scenarios, the ioss and pss resources from the platform device can IOSS PSS Fine. not be obtained and result in a invalid telemetry_plt_config. What is telemetry_plt_config? Internal data structure that holds platform config, maintained by the telemetry platform driver. This is also applicable to the platforms where the BIOS supports IPC1 device under debug configurations but IPC1 is disabled by user or the policy. This change allows user to know the reason for not seeing entries under /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. Cc: Matt Turner Cc: Len Brown Cc: Souvik Kumar Chakravarty Cc: Kuppuswamy Sathyanarayanan Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779 There should be not a blank line. OK. Acked-by: Matt Turner Signed-off-by: Rajneesh Bhardwaj +exit: + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n"); Completely useless. Device core does it in generic way. If i remove this print then perhaps there is no need of this patch. Reason to print this is that the platform driver / core driver does not show any error. In-fact they are even loaded in module table. OTOH, this debugfs interface fails. This is very confusing to the users if they check the lsmod output so i feel this print might help.
Re: [PATCH 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
On 26-Sep-18 7:23 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: The LTR values follow PCIE LTR encoding format and can be decoded as per https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf This adds support to translate the raw LTR values as read from the PMC to meaningful values in nanosecond units of time. +static void get_ltr_scale(u32 *val) What's wrong to return converted value? Actually the name should reflect what it does, ie *convert* value. I can change it as per your suggestion. +{ + /* +* As per PCIE specification supprting document supporting oops. Will fix. +* ECN_LatencyTolnReporting_14Aug08.pdf the Latency +* Tolerance Reporting data payload is encoded in a +* 3 bit scale and 10 bit value fields. Values are +* multiplied by the indicated scale to yield an absolute time +* value, expressible in a range from 1 nanosecond to +* 2^25*(2^10-1) = 34,326,183,936 nanoseconds. +* +* scale encoding is as follows: +* +* -- +* |scale factor| Multiplier (ns) | +* -- +* |0 | 1 | +* |1 | 32 | +* |2 | 1024| +* |3 | 32768 | +* |4 | 1048576 | +* |5 | 33554432| +* |6 | Invalid | +* |7 | Invalid | +* -- +*/ + if (*val > 5) { + *val = 0; + pr_warn("Invalid LTR scale factor.\n"); + } else { + *val = 1U << (5 * (*val)); + } +} + static int pmc_core_ltr_show(struct seq_file *s, void *unused) { struct pmc_dev *pmcdev = s->private; const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts; + u64 decoded_snoop_ltr = 0, decoded_non_snoop_ltr = 0; + union ltr_payload ltr_data; + u32 scale = 0; Redundant assignment. Ok int index; for (index = 0; map[index].name ; index++) { - seq_printf(s, "%-32s\tRAW LTR: 0x%x\n", + ltr_data.raw_data = pmc_core_reg_read(pmcdev, + map[index].bit_mask); + + if (ltr_data.bits.non_snoop_req) { + scale = ltr_data.bits.non_snoop_scale; + get_ltr_scale(); + decoded_non_snoop_ltr = + ltr_data.bits.non_snoop_val * scale; + } + + if (ltr_data.bits.snoop_req) { + scale = ltr_data.bits.snoop_scale; + get_ltr_scale(); + decoded_snoop_ltr = + ltr_data.bits.snoop_val * scale; + } + + seq_printf(s, "%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n", map[index].name, - pmc_core_reg_read(pmcdev, map[index].bit_mask)); + ltr_data.raw_data, + decoded_non_snoop_ltr, + decoded_snoop_ltr); + + decoded_snoop_ltr = decoded_non_snoop_ltr = 0; You may do this at the beginning of the loop and get rid of assignment in the definition block. Fine. } return 0; } +union ltr_payload { + u32 raw_data; + struct { + u32 snoop_val : 10; + u32 snoop_scale : 3; + u32 snoop_res : 2; + u32 snoop_req : 1; + u32 non_snoop_val : 10; + u32 non_snoop_scale : 3; + u32 non_snoop_res : 2; + u32 non_snoop_req : 1; + } bits; +}; Just use normal masks and shifts. I chose union over masks and shifts to reduce code size and ensured correct endian-ness. Just for my understanding, can you please let me know why you feel masks/shift are better suited here?
Re: [PATCH 3/4] platform/x86: intel_pmc_core: Decode Snoop / Non Snoop LTR
On 26-Sep-18 7:23 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: The LTR values follow PCIE LTR encoding format and can be decoded as per https://pcisig.com/sites/default/files/specification_documents/ECN_LatencyTolnReporting_14Aug08.pdf This adds support to translate the raw LTR values as read from the PMC to meaningful values in nanosecond units of time. +static void get_ltr_scale(u32 *val) What's wrong to return converted value? Actually the name should reflect what it does, ie *convert* value. I can change it as per your suggestion. +{ + /* +* As per PCIE specification supprting document supporting oops. Will fix. +* ECN_LatencyTolnReporting_14Aug08.pdf the Latency +* Tolerance Reporting data payload is encoded in a +* 3 bit scale and 10 bit value fields. Values are +* multiplied by the indicated scale to yield an absolute time +* value, expressible in a range from 1 nanosecond to +* 2^25*(2^10-1) = 34,326,183,936 nanoseconds. +* +* scale encoding is as follows: +* +* -- +* |scale factor| Multiplier (ns) | +* -- +* |0 | 1 | +* |1 | 32 | +* |2 | 1024| +* |3 | 32768 | +* |4 | 1048576 | +* |5 | 33554432| +* |6 | Invalid | +* |7 | Invalid | +* -- +*/ + if (*val > 5) { + *val = 0; + pr_warn("Invalid LTR scale factor.\n"); + } else { + *val = 1U << (5 * (*val)); + } +} + static int pmc_core_ltr_show(struct seq_file *s, void *unused) { struct pmc_dev *pmcdev = s->private; const struct pmc_bit_map *map = pmcdev->map->ltr_show_sts; + u64 decoded_snoop_ltr = 0, decoded_non_snoop_ltr = 0; + union ltr_payload ltr_data; + u32 scale = 0; Redundant assignment. Ok int index; for (index = 0; map[index].name ; index++) { - seq_printf(s, "%-32s\tRAW LTR: 0x%x\n", + ltr_data.raw_data = pmc_core_reg_read(pmcdev, + map[index].bit_mask); + + if (ltr_data.bits.non_snoop_req) { + scale = ltr_data.bits.non_snoop_scale; + get_ltr_scale(); + decoded_non_snoop_ltr = + ltr_data.bits.non_snoop_val * scale; + } + + if (ltr_data.bits.snoop_req) { + scale = ltr_data.bits.snoop_scale; + get_ltr_scale(); + decoded_snoop_ltr = + ltr_data.bits.snoop_val * scale; + } + + seq_printf(s, "%-24s\tRaw LTR: 0x%-16x\t Non-Snoop LTR (ns): %-16llu\t Snoop LTR (ns): %-16llu\n", map[index].name, - pmc_core_reg_read(pmcdev, map[index].bit_mask)); + ltr_data.raw_data, + decoded_non_snoop_ltr, + decoded_snoop_ltr); + + decoded_snoop_ltr = decoded_non_snoop_ltr = 0; You may do this at the beginning of the loop and get rid of assignment in the definition block. Fine. } return 0; } +union ltr_payload { + u32 raw_data; + struct { + u32 snoop_val : 10; + u32 snoop_scale : 3; + u32 snoop_res : 2; + u32 snoop_req : 1; + u32 non_snoop_val : 10; + u32 non_snoop_scale : 3; + u32 non_snoop_res : 2; + u32 non_snoop_req : 1; + } bits; +}; Just use normal masks and shifts. I chose union over masks and shifts to reduce code size and ensured correct endian-ness. Just for my understanding, can you please let me know why you feel masks/shift are better suited here?
Re: [PATCH 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info
On 26-Sep-18 7:18 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:06 PM Rajneesh Bhardwaj wrote: This adds support to show the Latency Tolerance Reporting for the IPs on the PCH as reported by the PMC. The format shown here is raw LTR data payload that can further be decoded as per the PCI specification. This also fixes some minor alignment issues in the header file by removing spaces and converting to tabs at some places. Thanks for the patch, my comments below. Hi Andy, Thanks for the review, my answers below. +static const struct pmc_bit_map spt_ltr_show_map[] = { + {"IP 0 : LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA}, + {"IP 1 : LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB}, + {"IP 2 : LTR_SATA",SPT_PMC_LTR_SATA}, + {"IP 3 : LTR_GIGABIT_ETHERNET",SPT_PMC_LTR_GBE}, + {"IP 4 : LTR_XHCI",SPT_PMC_LTR_XHCI}, + /* IP 5 is reserved */ + {"IP 6 : LTR_ME", SPT_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"IP 7 : LTR_EVA", SPT_PMC_LTR_EVA}, + {"IP 8 : LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC}, + {"IP 9 : LTR_HD_AUDIO",SPT_PMC_LTR_AZ}, + /* IP 10 is reserved */ + {"IP 11 : LTR_LPSS",SPT_PMC_LTR_LPSS}, + {"IP 12 : LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD}, + {"IP 13 : LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE}, + {"IP 14 : LTR_CAMERA", SPT_PMC_LTR_CAM}, + {"IP 15 : LTR_ESPI",SPT_PMC_LTR_ESPI}, + {"IP 16 : LTR_SCC", SPT_PMC_LTR_SCC}, + {"IP 17 : LTR_ISH", SPT_PMC_LTR_ISH}, + /* Below two cannot be for LTR_IGNORE */ + {"LTR_CURRENT_PLATFORM",SPT_PMC_LTR_CUR_PLT}, + {"LTR_AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT}, Before no map has this fancy "IP xx :" prefixes. Please, remove. The users of the driver often ask for IP Numbers while performing LTR_IGNORE operation so this is deliberately added. Please consider it. + {}, No need for comma Ok. + Redundant. OK +}; +static const struct pmc_bit_map cnp_ltr_show_map[] = { Same comments as above. +}; + debugfs_create_file("ltr_show", 0644, dir, pmcdev, + _core_ltr_fops); One line? IIRC, it was crossing the limit. I will check again and if possible would change it. #define NUM_RETRIES100 #define NUM_IP_IGN_ALLOWED 17 + blank line here. Sure. +#define SPT_PMC_LTR_CUR_PLT0x350
Re: [PATCH 1/4] platform/x86: intel_pmc_core: Show Latency Tolerance info
On 26-Sep-18 7:18 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:06 PM Rajneesh Bhardwaj wrote: This adds support to show the Latency Tolerance Reporting for the IPs on the PCH as reported by the PMC. The format shown here is raw LTR data payload that can further be decoded as per the PCI specification. This also fixes some minor alignment issues in the header file by removing spaces and converting to tabs at some places. Thanks for the patch, my comments below. Hi Andy, Thanks for the review, my answers below. +static const struct pmc_bit_map spt_ltr_show_map[] = { + {"IP 0 : LTR_SOUTHPORT_A", SPT_PMC_LTR_SPA}, + {"IP 1 : LTR_SOUTHPORT_B", SPT_PMC_LTR_SPB}, + {"IP 2 : LTR_SATA",SPT_PMC_LTR_SATA}, + {"IP 3 : LTR_GIGABIT_ETHERNET",SPT_PMC_LTR_GBE}, + {"IP 4 : LTR_XHCI",SPT_PMC_LTR_XHCI}, + /* IP 5 is reserved */ + {"IP 6 : LTR_ME", SPT_PMC_LTR_ME}, + /* EVA is Enterprise Value Add, doesn't really exist on PCH */ + {"IP 7 : LTR_EVA", SPT_PMC_LTR_EVA}, + {"IP 8 : LTR_SOUTHPORT_C", SPT_PMC_LTR_SPC}, + {"IP 9 : LTR_HD_AUDIO",SPT_PMC_LTR_AZ}, + /* IP 10 is reserved */ + {"IP 11 : LTR_LPSS",SPT_PMC_LTR_LPSS}, + {"IP 12 : LTR_SOUTHPORT_D", SPT_PMC_LTR_SPD}, + {"IP 13 : LTR_SOUTHPORT_E", SPT_PMC_LTR_SPE}, + {"IP 14 : LTR_CAMERA", SPT_PMC_LTR_CAM}, + {"IP 15 : LTR_ESPI",SPT_PMC_LTR_ESPI}, + {"IP 16 : LTR_SCC", SPT_PMC_LTR_SCC}, + {"IP 17 : LTR_ISH", SPT_PMC_LTR_ISH}, + /* Below two cannot be for LTR_IGNORE */ + {"LTR_CURRENT_PLATFORM",SPT_PMC_LTR_CUR_PLT}, + {"LTR_AGGREGATED_SYSTEM", SPT_PMC_LTR_CUR_ASLT}, Before no map has this fancy "IP xx :" prefixes. Please, remove. The users of the driver often ask for IP Numbers while performing LTR_IGNORE operation so this is deliberately added. Please consider it. + {}, No need for comma Ok. + Redundant. OK +}; +static const struct pmc_bit_map cnp_ltr_show_map[] = { Same comments as above. +}; + debugfs_create_file("ltr_show", 0644, dir, pmcdev, + _core_ltr_fops); One line? IIRC, it was crossing the limit. I will check again and if possible would change it. #define NUM_RETRIES100 #define NUM_IP_IGN_ALLOWED 17 + blank line here. Sure. +#define SPT_PMC_LTR_CUR_PLT0x350