[PATCH 0/3] Decrease startup time for amd_pmc

[PATCH 0/3] Decrease startup time for amd_pmc

[Mario Limonciello]

It was recently observed that one of the worst offenders for system
startup delays was the amd_pmc driver. With 5.18-rc1 and measured
with ftrace amd_pmc_probe spends about 116ms.  This is bonkers for
what the init actually does.

As a lot of the functionality that is slowing down startup is related
to debugging mechanisms or conditional code that is executed during
suspend, move it out of init.  This speeds up the startup to happen
in ~1ms.

This is done at the expense that the first suspend or first time debug
features are accessed it will be slower.  Those aren't critical path
for a user turning on the machine though, so this is a reasonable tradeoff.

Mario Limonciello (3):
  platform/x86: amd-pmc: Move SMU logging setup out of init
  platform/x86: amd-pmc: Move FCH init to first use
  platform/x86: amd-pmc: Avoid reading SMU version at probe time

 drivers/platform/x86/amd-pmc.c | 85 +++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH 1/3] platform/x86: amd-pmc: Move SMU logging setup out of init

[Mario Limonciello]

SMU logging is setup when the device is probed currently.

In analyzing boot performance it was observed that amd_pmc_probe is
taking ~116800us on startup on an OEM platform.  This is longer than
expected, and is caused by enabling SMU logging at startup.

As the SMU logging is only needed for debugging, initialize it only upon
use.  This decreases the time for amd_pmc_probe to ~28800us.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 54 ++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index e9d0dbbb2887..28e98e4649f1 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -162,6 +162,7 @@ static struct amd_pmc_dev pmc;
 static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
 static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
 static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
+static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev);
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
 {
@@ -319,6 +320,13 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
 
 static int get_metrics_table(struct amd_pmc_dev *pdev, struct smu_metrics *table)
 {
+	if (!pdev->smu_virt_addr) {
+		int ret = amd_pmc_setup_smu_logging(pdev);
+
+		if (ret)
+			return ret;
+	}
+
 	if (pdev->cpu_id == AMD_CPU_ID_PCO)
 		return -ENODEV;
 	memcpy_fromio(table, pdev->smu_virt_addr, sizeof(struct smu_metrics));
@@ -346,6 +354,13 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
 	struct smu_metrics table;
 	int idx;
 
+	if (!dev->active_ips) {
+		int ret = amd_pmc_setup_smu_logging(dev);
+
+		if (ret)
+			return ret;
+	}
+
 	if (get_metrics_table(dev, &table))
 		return -EINVAL;
 
@@ -447,25 +462,32 @@ static inline void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
 
 static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
 {
-	u32 phys_addr_low, phys_addr_hi;
-	u64 smu_phys_addr;
-
-	if (dev->cpu_id == AMD_CPU_ID_PCO)
+	if (dev->cpu_id == AMD_CPU_ID_PCO) {
+		dev_warn_once(dev->dev, "SMU debugging info not supported on this platform\n");
 		return -EINVAL;
+	}
 
 	/* Get Active devices list from SMU */
-	amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
+	if (!dev->active_ips)
+		amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
 
 	/* Get dram address */
-	amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, 1);
-	amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, 1);
-	smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
-
-	dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr, sizeof(struct smu_metrics));
-	if (!dev->smu_virt_addr)
-		return -ENOMEM;
+	if (!dev->smu_virt_addr) {
+		u32 phys_addr_low, phys_addr_hi;
+		u64 smu_phys_addr;
+
+		amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, 1);
+		amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, 1);
+		smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
+
+		dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr,
+						  sizeof(struct smu_metrics));
+		if (!dev->smu_virt_addr)
+			return -ENOMEM;
+	}
 
 	/* Start the logging */
+	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_RESET, 0);
 	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, 0);
 
 	return 0;
@@ -634,8 +656,7 @@ static void amd_pmc_s2idle_prepare(void)
 	u32 arg = 1;
 
 	/* Reset and Start SMU logging - to monitor the s0i3 stats */
-	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
-	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
+	amd_pmc_setup_smu_logging(pdev);
 
 	/* Activate CZN specific RTC functionality */
 	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
@@ -846,11 +867,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 		goto err_pci_dev_put;
 	}
 
-	/* Use SMU to get the s0i3 debug stats */
-	err = amd_pmc_setup_smu_logging(dev);
-	if (err)
-		dev_err(dev->dev, "SMU debugging info not supported on this platform\n");
-
 	if (enable_stb && dev->cpu_id == AMD_CPU_ID_YC) {
 		err = amd_pmc_s2d_init(dev);
 		if (err)
-- 
2.34.1

[PATCH 2/3] platform/x86: amd-pmc: Move FCH init to first use

[Mario Limonciello]

FCH address is accessed only when looking at s0ix stats.

As this is unnecessary for initialization, move it ito the first
time stats are accessed from sysfs.  This descrease startup time
by about 200us.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 28e98e4649f1..66be2601866b 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -390,6 +390,17 @@ static int s0ix_stats_show(struct seq_file *s, void *unused)
 	struct amd_pmc_dev *dev = s->private;
 	u64 entry_time, exit_time, residency;
 
+	/* Use FCH registers to get the S0ix stats */
+	if (!dev->fch_virt_addr) {
+		u32 base_addr_lo = FCH_BASE_PHY_ADDR_LOW;
+		u32 base_addr_hi = FCH_BASE_PHY_ADDR_HIGH;
+		u64 fch_phys_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
+
+		dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
+		if (!dev->fch_virt_addr)
+			return -ENOMEM;
+	}
+
 	entry_time = ioread32(dev->fch_virt_addr + FCH_S0I3_ENTRY_TIME_H_OFFSET);
 	entry_time = entry_time << 32 | ioread32(dev->fch_virt_addr + FCH_S0I3_ENTRY_TIME_L_OFFSET);
 
@@ -803,7 +814,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	struct amd_pmc_dev *dev = &pmc;
 	struct pci_dev *rdev;
 	u32 base_addr_lo, base_addr_hi;
-	u64 base_addr, fch_phys_addr;
+	u64 base_addr;
 	int err;
 	u32 val;
 
@@ -857,16 +868,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 
 	mutex_init(&dev->lock);
 
-	/* Use FCH registers to get the S0ix stats */
-	base_addr_lo = FCH_BASE_PHY_ADDR_LOW;
-	base_addr_hi = FCH_BASE_PHY_ADDR_HIGH;
-	fch_phys_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
-	dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
-	if (!dev->fch_virt_addr) {
-		err = -ENOMEM;
-		goto err_pci_dev_put;
-	}
-
 	if (enable_stb && dev->cpu_id == AMD_CPU_ID_YC) {
 		err = amd_pmc_s2d_init(dev);
 		if (err)
-- 
2.34.1

[PATCH 3/3] platform/x86: amd-pmc: Avoid reading SMU version at probe time

[Mario Limonciello]

Currently the SMU version only used to determine whether the SMU supports
reading the idle mask. To speed up startup, move it to the first time
the idle mask is read.

This decreases the startup time from ~28500us to 100us.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/platform/x86/amd-pmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 66be2601866b..177c0fe0bae8 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -425,6 +425,13 @@ static int amd_pmc_idlemask_show(struct seq_file *s, void *unused)
 	struct amd_pmc_dev *dev = s->private;
 	int rc;
 
+	/* we haven't yet read SMU version */
+	if (!dev->major) {
+		rc = amd_pmc_get_smu_version(dev);
+		if (rc)
+			return rc;
+	}
+
 	if (dev->major > 56 || (dev->major >= 55 && dev->minor >= 37)) {
 		rc = amd_pmc_idlemask_read(dev, NULL, s);
 		if (rc)
@@ -874,7 +881,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	amd_pmc_get_smu_version(dev);
 	platform_set_drvdata(pdev, dev);
 	err = acpi_register_lps0_dev(&amd_pmc_s2idle_dev_ops);
 	if (err)
-- 
2.34.1

Re: [PATCH 1/3] platform/x86: amd-pmc: Move SMU logging setup out of init

[Hans de Goede]

Hi,

On 4/6/22 22:26, Mario Limonciello wrote:
> SMU logging is setup when the device is probed currently.
> 
> In analyzing boot performance it was observed that amd_pmc_probe is
> taking ~116800us on startup on an OEM platform.  This is longer than
> expected, and is caused by enabling SMU logging at startup.
> 
> As the SMU logging is only needed for debugging, initialize it only upon
> use.  This decreases the time for amd_pmc_probe to ~28800us.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/platform/x86/amd-pmc.c | 54 ++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index e9d0dbbb2887..28e98e4649f1 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -162,6 +162,7 @@ static struct amd_pmc_dev pmc;
>  static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
>  static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
>  static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
> +static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev);
>  
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
>  {
> @@ -319,6 +320,13 @@ static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
>  
>  static int get_metrics_table(struct amd_pmc_dev *pdev, struct smu_metrics *table)
>  {
> +	if (!pdev->smu_virt_addr) {
> +		int ret = amd_pmc_setup_smu_logging(pdev);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (pdev->cpu_id == AMD_CPU_ID_PCO)
>  		return -ENODEV;
>  	memcpy_fromio(table, pdev->smu_virt_addr, sizeof(struct smu_metrics));
> @@ -346,6 +354,13 @@ static int smu_fw_info_show(struct seq_file *s, void *unused)
>  	struct smu_metrics table;
>  	int idx;
>  
> +	if (!dev->active_ips) {
> +		int ret = amd_pmc_setup_smu_logging(dev);
> +
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (get_metrics_table(dev, &table))
>  		return -EINVAL;
>  

This addition seems unnecessary, since it is immediately followed by get_metrics_table()
which will already call amd_pmc_setup_smu_logging() if not done already ?

Otherwise the series looks good to me, so
with this dropped you can add:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

To v2 of the series.

Regards,

Hans


> @@ -447,25 +462,32 @@ static inline void amd_pmc_dbgfs_unregister(struct amd_pmc_dev *dev)
>  
>  static int amd_pmc_setup_smu_logging(struct amd_pmc_dev *dev)
>  {
> -	u32 phys_addr_low, phys_addr_hi;
> -	u64 smu_phys_addr;
> -
> -	if (dev->cpu_id == AMD_CPU_ID_PCO)
> +	if (dev->cpu_id == AMD_CPU_ID_PCO) {
> +		dev_warn_once(dev->dev, "SMU debugging info not supported on this platform\n");
>  		return -EINVAL;
> +	}
>  
>  	/* Get Active devices list from SMU */
> -	amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
> +	if (!dev->active_ips)
> +		amd_pmc_send_cmd(dev, 0, &dev->active_ips, SMU_MSG_GET_SUP_CONSTRAINTS, 1);
>  
>  	/* Get dram address */
> -	amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, 1);
> -	amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, 1);
> -	smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
> -
> -	dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr, sizeof(struct smu_metrics));
> -	if (!dev->smu_virt_addr)
> -		return -ENOMEM;
> +	if (!dev->smu_virt_addr) {
> +		u32 phys_addr_low, phys_addr_hi;
> +		u64 smu_phys_addr;
> +
> +		amd_pmc_send_cmd(dev, 0, &phys_addr_low, SMU_MSG_LOG_GETDRAM_ADDR_LO, 1);
> +		amd_pmc_send_cmd(dev, 0, &phys_addr_hi, SMU_MSG_LOG_GETDRAM_ADDR_HI, 1);
> +		smu_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
> +
> +		dev->smu_virt_addr = devm_ioremap(dev->dev, smu_phys_addr,
> +						  sizeof(struct smu_metrics));
> +		if (!dev->smu_virt_addr)
> +			return -ENOMEM;
> +	}
>  
>  	/* Start the logging */
> +	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_RESET, 0);
>  	amd_pmc_send_cmd(dev, 0, NULL, SMU_MSG_LOG_START, 0);
>  
>  	return 0;
> @@ -634,8 +656,7 @@ static void amd_pmc_s2idle_prepare(void)
>  	u32 arg = 1;
>  
>  	/* Reset and Start SMU logging - to monitor the s0i3 stats */
> -	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
> -	amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
> +	amd_pmc_setup_smu_logging(pdev);
>  
>  	/* Activate CZN specific RTC functionality */
>  	if (pdev->cpu_id == AMD_CPU_ID_CZN) {
> @@ -846,11 +867,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  		goto err_pci_dev_put;
>  	}
>  
> -	/* Use SMU to get the s0i3 debug stats */
> -	err = amd_pmc_setup_smu_logging(dev);
> -	if (err)
> -		dev_err(dev->dev, "SMU debugging info not supported on this platform\n");
> -
>  	if (enable_stb && dev->cpu_id == AMD_CPU_ID_YC) {
>  		err = amd_pmc_s2d_init(dev);
>  		if (err)