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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
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)
