[AMD Official Use Only] > -----Original Message----- > From: Hans de Goede <hdegoede@xxxxxxxxxx> > Sent: Monday, April 11, 2022 09:56 > To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Mark Gross > <mgross@xxxxxxxxxxxxxxx>; open list:X86 PLATFORM DRIVERS <platform-driver- > x86@xxxxxxxxxxxxxxx> > Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx>; Goswami, Sanket > <Sanket.Goswami@xxxxxxx>; rrangel@xxxxxxxxxxxx > Subject: Re: [PATCH v2 1/3] platform/x86: amd-pmc: Move SMU logging setup > out of init > > Hi Mario, > > On 4/11/22 16:38, 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. > > > > Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx> > > --- > > v1->v2: > > * Drop extra check for dev->active_ips as already validated in > get_metrics_table > > * Add tag > > Thank you for your patch-series, I've applied the series to my > review-hans branch: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers- > x86.git%2Flog%2F%3Fh%3Dreview- > hans&data=04%7C01%7Cmario.limonciello%40amd.com%7Ca66415c73ed > 04611830c08da1bcb69d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0 > %7C637852857715985040%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata > =hZkKrgZOzSgTIUe%2BGJK7wGpq%2BoCgB6j9SH%2BUT2rok9M%3D&reser > ved=0 > > Note I had to resolve some trivial conflicts manually because your > base did not include the "platform/x86: amd-pmc: Fix compilation > without CONFIG_SUSPEND" patch (1), please double check the results. Ah thanks. I had done it relative to platform-x86/for-next (as of b49f72e7f96d4ed147447428f2ae5b4cea598ca7), didn't realize it wasn't yet applied there until you said this. > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. > > Regards, > > Hans > > > 1) To avoid this in the future please use the: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers- > x86.git%2Flog%2F%3Fh%3Dreview- > hans&data=04%7C01%7Cmario.limonciello%40amd.com%7Ca66415c73ed > 04611830c08da1bcb69d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0 > %7C637852857715985040%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata > =hZkKrgZOzSgTIUe%2BGJK7wGpq%2BoCgB6j9SH%2BUT2rok9M%3D&reser > ved=0 > branch as base in the future. > > > > > > > > > > drivers/platform/x86/amd-pmc.c | 47 ++++++++++++++++++++-------------- > > 1 file changed, 28 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd- > pmc.c > > index e9d0dbbb2887..103ba0729b2a 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)); > > @@ -447,25 +455,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 +649,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 +860,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)
