[PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Shyam Sundar S K]

amd_pmc_get_dram_size() is used to get the DRAM size information. But in
the current code, mailbox command to get the DRAM size info is sent based
on the values of dev->major and dev->minor.

But dev->major and dev->minor will have either junk or zero assigned to
them until at least once a call to amd_pmc_get_smu_version() is made which
ideally populates dev->major and dev->minor.

Add a missing amd_pmc_get_smu_version() call to amd_pmc_get_dram_size().

Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
based on review-ilpo, can be added on top of recent stb changes

 drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 1424c03c1f03..92adf4523736 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
 {
 	int ret;
 
+	ret = amd_pmc_get_smu_version(dev);
+	if (ret)
+		return ret;
+
 	switch (dev->cpu_id) {
 	case AMD_CPU_ID_YC:
 		if (!(dev->major > 90 || (dev->major == 90 && dev->minor > 39))) {
-- 
2.25.1

[PATCH 2/2] platform/x86/amd/pmc: Move structures to pmc header

[Shyam Sundar S K]

As we have a separate header for amd_pmc driver, move the common strutures,
enums, and macros to the header file.

Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
based on review-ilpo, can be added on top of recent stb changes

 drivers/platform/x86/amd/pmc/pmc.c | 103 -----------------------------
 drivers/platform/x86/amd/pmc/pmc.h | 103 +++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 103 deletions(-)

diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
index 92adf4523736..943fb85af91d 100644
--- a/drivers/platform/x86/amd/pmc/pmc.c
+++ b/drivers/platform/x86/amd/pmc/pmc.c
@@ -30,109 +30,6 @@
 
 #include "pmc.h"
 
-/* SMU communication registers */
-#define AMD_PMC_REGISTER_MESSAGE	0x538
-#define AMD_PMC_REGISTER_RESPONSE	0x980
-#define AMD_PMC_REGISTER_ARGUMENT	0x9BC
-
-/* PMC Scratch Registers */
-#define AMD_PMC_SCRATCH_REG_CZN		0x94
-#define AMD_PMC_SCRATCH_REG_YC		0xD14
-
-/* STB Registers */
-#define AMD_PMC_STB_PMI_0		0x03E30600
-#define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
-#define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
-#define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
-#define AMD_PMC_STB_DUMMY_PC		0xC6000007
-
-/* STB S2D(Spill to DRAM) has different message port offset */
-#define AMD_S2D_REGISTER_MESSAGE	0xA20
-#define AMD_S2D_REGISTER_RESPONSE	0xA80
-#define AMD_S2D_REGISTER_ARGUMENT	0xA88
-
-/* STB Spill to DRAM Parameters */
-#define S2D_TELEMETRY_BYTES_MAX		0x100000
-#define S2D_RSVD_RAM_SPACE		0x100000
-#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
-
-/* STB Spill to DRAM Message Definition */
-#define STB_FORCE_FLUSH_DATA		0xCF
-
-/* Base address of SMU for mapping physical address to virtual address */
-#define AMD_PMC_MAPPING_SIZE		0x01000
-#define AMD_PMC_BASE_ADDR_OFFSET	0x10000
-#define AMD_PMC_BASE_ADDR_LO		0x13B102E8
-#define AMD_PMC_BASE_ADDR_HI		0x13B102EC
-#define AMD_PMC_BASE_ADDR_LO_MASK	GENMASK(15, 0)
-#define AMD_PMC_BASE_ADDR_HI_MASK	GENMASK(31, 20)
-
-/* SMU Response Codes */
-#define AMD_PMC_RESULT_OK                    0x01
-#define AMD_PMC_RESULT_CMD_REJECT_BUSY       0xFC
-#define AMD_PMC_RESULT_CMD_REJECT_PREREQ     0xFD
-#define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
-#define AMD_PMC_RESULT_FAILED                0xFF
-
-/* FCH SSC Registers */
-#define FCH_S0I3_ENTRY_TIME_L_OFFSET	0x30
-#define FCH_S0I3_ENTRY_TIME_H_OFFSET	0x34
-#define FCH_S0I3_EXIT_TIME_L_OFFSET	0x38
-#define FCH_S0I3_EXIT_TIME_H_OFFSET	0x3C
-#define FCH_SSC_MAPPING_SIZE		0x800
-#define FCH_BASE_PHY_ADDR_LOW		0xFED81100
-#define FCH_BASE_PHY_ADDR_HIGH		0x00000000
-
-/* SMU Message Definations */
-#define SMU_MSG_GETSMUVERSION		0x02
-#define SMU_MSG_LOG_GETDRAM_ADDR_HI	0x04
-#define SMU_MSG_LOG_GETDRAM_ADDR_LO	0x05
-#define SMU_MSG_LOG_START		0x06
-#define SMU_MSG_LOG_RESET		0x07
-#define SMU_MSG_LOG_DUMP_DATA		0x08
-#define SMU_MSG_GET_SUP_CONSTRAINTS	0x09
-/* List of supported CPU ids */
-#define AMD_CPU_ID_RV			0x15D0
-#define AMD_CPU_ID_RN			0x1630
-#define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
-#define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
-#define AMD_CPU_ID_YC			0x14B5
-#define AMD_CPU_ID_CB			0x14D8
-#define AMD_CPU_ID_PS			0x14E8
-#define AMD_CPU_ID_SP			0x14A4
-#define PCI_DEVICE_ID_AMD_1AH_M20H_ROOT 0x1507
-
-#define PMC_MSG_DELAY_MIN_US		50
-#define RESPONSE_REGISTER_LOOP_MAX	20000
-
-#define DELAY_MIN_US		2000
-#define DELAY_MAX_US		3000
-#define FIFO_SIZE		4096
-
-enum amd_pmc_def {
-	MSG_TEST = 0x01,
-	MSG_OS_HINT_PCO,
-	MSG_OS_HINT_RN,
-};
-
-enum s2d_arg {
-	S2D_TELEMETRY_SIZE = 0x01,
-	S2D_PHYS_ADDR_LOW,
-	S2D_PHYS_ADDR_HIGH,
-	S2D_NUM_SAMPLES,
-	S2D_DRAM_SIZE,
-};
-
-struct amd_pmc_stb_v2_data {
-	size_t size;
-	u8 data[] __counted_by(size);
-};
-
-struct amd_pmc_bit_map {
-	const char *name;
-	u32 bit_mask;
-};
-
 static const struct amd_pmc_bit_map soc15_ip_blk[] = {
 	{"DISPLAY",	BIT(0)},
 	{"CPU",		BIT(1)},
diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
index 12728eedecda..704256b07ef4 100644
--- a/drivers/platform/x86/amd/pmc/pmc.h
+++ b/drivers/platform/x86/amd/pmc/pmc.h
@@ -14,6 +14,109 @@
 #include <linux/types.h>
 #include <linux/mutex.h>
 
+/* SMU communication registers */
+#define AMD_PMC_REGISTER_MESSAGE	0x538
+#define AMD_PMC_REGISTER_RESPONSE	0x980
+#define AMD_PMC_REGISTER_ARGUMENT	0x9BC
+
+/* PMC Scratch Registers */
+#define AMD_PMC_SCRATCH_REG_CZN		0x94
+#define AMD_PMC_SCRATCH_REG_YC		0xD14
+
+/* STB Registers */
+#define AMD_PMC_STB_PMI_0		0x03E30600
+#define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
+#define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
+#define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
+#define AMD_PMC_STB_DUMMY_PC		0xC6000007
+
+/* STB S2D(Spill to DRAM) has different message port offset */
+#define AMD_S2D_REGISTER_MESSAGE	0xA20
+#define AMD_S2D_REGISTER_RESPONSE	0xA80
+#define AMD_S2D_REGISTER_ARGUMENT	0xA88
+
+/* STB Spill to DRAM Parameters */
+#define S2D_TELEMETRY_BYTES_MAX		0x100000
+#define S2D_RSVD_RAM_SPACE			0x100000
+#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
+
+/* STB Spill to DRAM Message Definition */
+#define STB_FORCE_FLUSH_DATA		0xCF
+
+/* Base address of SMU for mapping physical address to virtual address */
+#define AMD_PMC_MAPPING_SIZE		0x01000
+#define AMD_PMC_BASE_ADDR_OFFSET	0x10000
+#define AMD_PMC_BASE_ADDR_LO		0x13B102E8
+#define AMD_PMC_BASE_ADDR_HI		0x13B102EC
+#define AMD_PMC_BASE_ADDR_LO_MASK	GENMASK(15, 0)
+#define AMD_PMC_BASE_ADDR_HI_MASK	GENMASK(31, 20)
+
+/* SMU Response Codes */
+#define AMD_PMC_RESULT_OK                    0x01
+#define AMD_PMC_RESULT_CMD_REJECT_BUSY       0xFC
+#define AMD_PMC_RESULT_CMD_REJECT_PREREQ     0xFD
+#define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
+#define AMD_PMC_RESULT_FAILED                0xFF
+
+/* FCH SSC Registers */
+#define FCH_S0I3_ENTRY_TIME_L_OFFSET	0x30
+#define FCH_S0I3_ENTRY_TIME_H_OFFSET	0x34
+#define FCH_S0I3_EXIT_TIME_L_OFFSET	0x38
+#define FCH_S0I3_EXIT_TIME_H_OFFSET	0x3C
+#define FCH_SSC_MAPPING_SIZE		0x800
+#define FCH_BASE_PHY_ADDR_LOW		0xFED81100
+#define FCH_BASE_PHY_ADDR_HIGH		0x00000000
+
+/* SMU Message Definations */
+#define SMU_MSG_GETSMUVERSION		0x02
+#define SMU_MSG_LOG_GETDRAM_ADDR_HI	0x04
+#define SMU_MSG_LOG_GETDRAM_ADDR_LO	0x05
+#define SMU_MSG_LOG_START		0x06
+#define SMU_MSG_LOG_RESET		0x07
+#define SMU_MSG_LOG_DUMP_DATA		0x08
+#define SMU_MSG_GET_SUP_CONSTRAINTS	0x09
+/* List of supported CPU ids */
+#define AMD_CPU_ID_RV			0x15D0
+#define AMD_CPU_ID_RN			0x1630
+#define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
+#define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
+#define AMD_CPU_ID_YC			0x14B5
+#define AMD_CPU_ID_CB			0x14D8
+#define AMD_CPU_ID_PS			0x14E8
+#define AMD_CPU_ID_SP			0x14A4
+#define PCI_DEVICE_ID_AMD_1AH_M20H_ROOT 0x1507
+
+#define PMC_MSG_DELAY_MIN_US		50
+#define RESPONSE_REGISTER_LOOP_MAX	20000
+
+#define DELAY_MIN_US		2000
+#define DELAY_MAX_US		3000
+#define FIFO_SIZE		4096
+
+enum amd_pmc_def {
+	MSG_TEST = 0x01,
+	MSG_OS_HINT_PCO,
+	MSG_OS_HINT_RN,
+};
+
+enum s2d_arg {
+	S2D_TELEMETRY_SIZE = 0x01,
+	S2D_PHYS_ADDR_LOW,
+	S2D_PHYS_ADDR_HIGH,
+	S2D_NUM_SAMPLES,
+	S2D_DRAM_SIZE,
+};
+
+struct amd_pmc_stb_v2_data {
+	size_t size;
+	u8 data[] __counted_by(size);
+};
+
+struct amd_pmc_bit_map {
+	const char *name;
+	u32 bit_mask;
+};
+
 struct amd_pmc_dev {
 	void __iomem *regbase;
 	void __iomem *smu_virt_addr;
-- 
2.25.1

Re: [PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Mario Limonciello]

On 10/9/2023 09:47, Shyam Sundar S K wrote:
> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
> the current code, mailbox command to get the DRAM size info is sent based
> on the values of dev->major and dev->minor.
> 
> But dev->major and dev->minor will have either junk or zero assigned to
> them until at least once a call to amd_pmc_get_smu_version() is made which
> ideally populates dev->major and dev->minor.
> 
> Add a missing amd_pmc_get_smu_version() call to amd_pmc_get_dram_size().
> 
> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> based on review-ilpo, can be added on top of recent stb changes

Hmm, so this is actually going to mean that we end up with a round trip 
call to the SMU as part of probe to get the version when STB is initialized.

I think that's going to translate to a longer amd_pmc startup time, 
which could be problematic if a platform leaves STB enabled by default.

At a minimum I would suggest to only do the version check when it's one 
that needs it (IE AMD_CPU_ID_YC right now).  But otherwise this s2d 
initialization can't happen the first time it's used?

> 
>   drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 1424c03c1f03..92adf4523736 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
>   {
>   	int ret;
>   
> +	ret = amd_pmc_get_smu_version(dev);
> +	if (ret)
> +		return ret;
> +
>   	switch (dev->cpu_id) {
>   	case AMD_CPU_ID_YC:
>   		if (!(dev->major > 90 || (dev->major == 90 && dev->minor > 39))) {

Re: [PATCH 2/2] platform/x86/amd/pmc: Move structures to pmc header

[Mario Limonciello]

On 10/9/2023 09:47, Shyam Sundar S K wrote:
> As we have a separate header for amd_pmc driver, move the common strutures,
> enums, and macros to the header file.
> 
> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> based on review-ilpo, can be added on top of recent stb changes

This also splits it up nicely to be able to potentially re-organize the 
driver more logically.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> 
>   drivers/platform/x86/amd/pmc/pmc.c | 103 -----------------------------
>   drivers/platform/x86/amd/pmc/pmc.h | 103 +++++++++++++++++++++++++++++
>   2 files changed, 103 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 92adf4523736..943fb85af91d 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -30,109 +30,6 @@
>   
>   #include "pmc.h"
>   
> -/* SMU communication registers */
> -#define AMD_PMC_REGISTER_MESSAGE	0x538
> -#define AMD_PMC_REGISTER_RESPONSE	0x980
> -#define AMD_PMC_REGISTER_ARGUMENT	0x9BC
> -
> -/* PMC Scratch Registers */
> -#define AMD_PMC_SCRATCH_REG_CZN		0x94
> -#define AMD_PMC_SCRATCH_REG_YC		0xD14
> -
> -/* STB Registers */
> -#define AMD_PMC_STB_PMI_0		0x03E30600
> -#define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
> -#define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
> -#define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
> -#define AMD_PMC_STB_DUMMY_PC		0xC6000007
> -
> -/* STB S2D(Spill to DRAM) has different message port offset */
> -#define AMD_S2D_REGISTER_MESSAGE	0xA20
> -#define AMD_S2D_REGISTER_RESPONSE	0xA80
> -#define AMD_S2D_REGISTER_ARGUMENT	0xA88
> -
> -/* STB Spill to DRAM Parameters */
> -#define S2D_TELEMETRY_BYTES_MAX		0x100000
> -#define S2D_RSVD_RAM_SPACE		0x100000
> -#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
> -
> -/* STB Spill to DRAM Message Definition */
> -#define STB_FORCE_FLUSH_DATA		0xCF
> -
> -/* Base address of SMU for mapping physical address to virtual address */
> -#define AMD_PMC_MAPPING_SIZE		0x01000
> -#define AMD_PMC_BASE_ADDR_OFFSET	0x10000
> -#define AMD_PMC_BASE_ADDR_LO		0x13B102E8
> -#define AMD_PMC_BASE_ADDR_HI		0x13B102EC
> -#define AMD_PMC_BASE_ADDR_LO_MASK	GENMASK(15, 0)
> -#define AMD_PMC_BASE_ADDR_HI_MASK	GENMASK(31, 20)
> -
> -/* SMU Response Codes */
> -#define AMD_PMC_RESULT_OK                    0x01
> -#define AMD_PMC_RESULT_CMD_REJECT_BUSY       0xFC
> -#define AMD_PMC_RESULT_CMD_REJECT_PREREQ     0xFD
> -#define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
> -#define AMD_PMC_RESULT_FAILED                0xFF
> -
> -/* FCH SSC Registers */
> -#define FCH_S0I3_ENTRY_TIME_L_OFFSET	0x30
> -#define FCH_S0I3_ENTRY_TIME_H_OFFSET	0x34
> -#define FCH_S0I3_EXIT_TIME_L_OFFSET	0x38
> -#define FCH_S0I3_EXIT_TIME_H_OFFSET	0x3C
> -#define FCH_SSC_MAPPING_SIZE		0x800
> -#define FCH_BASE_PHY_ADDR_LOW		0xFED81100
> -#define FCH_BASE_PHY_ADDR_HIGH		0x00000000
> -
> -/* SMU Message Definations */
> -#define SMU_MSG_GETSMUVERSION		0x02
> -#define SMU_MSG_LOG_GETDRAM_ADDR_HI	0x04
> -#define SMU_MSG_LOG_GETDRAM_ADDR_LO	0x05
> -#define SMU_MSG_LOG_START		0x06
> -#define SMU_MSG_LOG_RESET		0x07
> -#define SMU_MSG_LOG_DUMP_DATA		0x08
> -#define SMU_MSG_GET_SUP_CONSTRAINTS	0x09
> -/* List of supported CPU ids */
> -#define AMD_CPU_ID_RV			0x15D0
> -#define AMD_CPU_ID_RN			0x1630
> -#define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
> -#define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
> -#define AMD_CPU_ID_YC			0x14B5
> -#define AMD_CPU_ID_CB			0x14D8
> -#define AMD_CPU_ID_PS			0x14E8
> -#define AMD_CPU_ID_SP			0x14A4
> -#define PCI_DEVICE_ID_AMD_1AH_M20H_ROOT 0x1507
> -
> -#define PMC_MSG_DELAY_MIN_US		50
> -#define RESPONSE_REGISTER_LOOP_MAX	20000
> -
> -#define DELAY_MIN_US		2000
> -#define DELAY_MAX_US		3000
> -#define FIFO_SIZE		4096
> -
> -enum amd_pmc_def {
> -	MSG_TEST = 0x01,
> -	MSG_OS_HINT_PCO,
> -	MSG_OS_HINT_RN,
> -};
> -
> -enum s2d_arg {
> -	S2D_TELEMETRY_SIZE = 0x01,
> -	S2D_PHYS_ADDR_LOW,
> -	S2D_PHYS_ADDR_HIGH,
> -	S2D_NUM_SAMPLES,
> -	S2D_DRAM_SIZE,
> -};
> -
> -struct amd_pmc_stb_v2_data {
> -	size_t size;
> -	u8 data[] __counted_by(size);
> -};
> -
> -struct amd_pmc_bit_map {
> -	const char *name;
> -	u32 bit_mask;
> -};
> -
>   static const struct amd_pmc_bit_map soc15_ip_blk[] = {
>   	{"DISPLAY",	BIT(0)},
>   	{"CPU",		BIT(1)},
> diff --git a/drivers/platform/x86/amd/pmc/pmc.h b/drivers/platform/x86/amd/pmc/pmc.h
> index 12728eedecda..704256b07ef4 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.h
> +++ b/drivers/platform/x86/amd/pmc/pmc.h
> @@ -14,6 +14,109 @@
>   #include <linux/types.h>
>   #include <linux/mutex.h>
>   
> +/* SMU communication registers */
> +#define AMD_PMC_REGISTER_MESSAGE	0x538
> +#define AMD_PMC_REGISTER_RESPONSE	0x980
> +#define AMD_PMC_REGISTER_ARGUMENT	0x9BC
> +
> +/* PMC Scratch Registers */
> +#define AMD_PMC_SCRATCH_REG_CZN		0x94
> +#define AMD_PMC_SCRATCH_REG_YC		0xD14
> +
> +/* STB Registers */
> +#define AMD_PMC_STB_PMI_0		0x03E30600
> +#define AMD_PMC_STB_S2IDLE_PREPARE	0xC6000001
> +#define AMD_PMC_STB_S2IDLE_RESTORE	0xC6000002
> +#define AMD_PMC_STB_S2IDLE_CHECK	0xC6000003
> +#define AMD_PMC_STB_DUMMY_PC		0xC6000007
> +
> +/* STB S2D(Spill to DRAM) has different message port offset */
> +#define AMD_S2D_REGISTER_MESSAGE	0xA20
> +#define AMD_S2D_REGISTER_RESPONSE	0xA80
> +#define AMD_S2D_REGISTER_ARGUMENT	0xA88
> +
> +/* STB Spill to DRAM Parameters */
> +#define S2D_TELEMETRY_BYTES_MAX		0x100000
> +#define S2D_RSVD_RAM_SPACE			0x100000
> +#define S2D_TELEMETRY_DRAMBYTES_MAX	0x1000000
> +
> +/* STB Spill to DRAM Message Definition */
> +#define STB_FORCE_FLUSH_DATA		0xCF
> +
> +/* Base address of SMU for mapping physical address to virtual address */
> +#define AMD_PMC_MAPPING_SIZE		0x01000
> +#define AMD_PMC_BASE_ADDR_OFFSET	0x10000
> +#define AMD_PMC_BASE_ADDR_LO		0x13B102E8
> +#define AMD_PMC_BASE_ADDR_HI		0x13B102EC
> +#define AMD_PMC_BASE_ADDR_LO_MASK	GENMASK(15, 0)
> +#define AMD_PMC_BASE_ADDR_HI_MASK	GENMASK(31, 20)
> +
> +/* SMU Response Codes */
> +#define AMD_PMC_RESULT_OK                    0x01
> +#define AMD_PMC_RESULT_CMD_REJECT_BUSY       0xFC
> +#define AMD_PMC_RESULT_CMD_REJECT_PREREQ     0xFD
> +#define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
> +#define AMD_PMC_RESULT_FAILED                0xFF
> +
> +/* FCH SSC Registers */
> +#define FCH_S0I3_ENTRY_TIME_L_OFFSET	0x30
> +#define FCH_S0I3_ENTRY_TIME_H_OFFSET	0x34
> +#define FCH_S0I3_EXIT_TIME_L_OFFSET	0x38
> +#define FCH_S0I3_EXIT_TIME_H_OFFSET	0x3C
> +#define FCH_SSC_MAPPING_SIZE		0x800
> +#define FCH_BASE_PHY_ADDR_LOW		0xFED81100
> +#define FCH_BASE_PHY_ADDR_HIGH		0x00000000
> +
> +/* SMU Message Definations */
> +#define SMU_MSG_GETSMUVERSION		0x02
> +#define SMU_MSG_LOG_GETDRAM_ADDR_HI	0x04
> +#define SMU_MSG_LOG_GETDRAM_ADDR_LO	0x05
> +#define SMU_MSG_LOG_START		0x06
> +#define SMU_MSG_LOG_RESET		0x07
> +#define SMU_MSG_LOG_DUMP_DATA		0x08
> +#define SMU_MSG_GET_SUP_CONSTRAINTS	0x09
> +/* List of supported CPU ids */
> +#define AMD_CPU_ID_RV			0x15D0
> +#define AMD_CPU_ID_RN			0x1630
> +#define AMD_CPU_ID_PCO			AMD_CPU_ID_RV
> +#define AMD_CPU_ID_CZN			AMD_CPU_ID_RN
> +#define AMD_CPU_ID_YC			0x14B5
> +#define AMD_CPU_ID_CB			0x14D8
> +#define AMD_CPU_ID_PS			0x14E8
> +#define AMD_CPU_ID_SP			0x14A4
> +#define PCI_DEVICE_ID_AMD_1AH_M20H_ROOT 0x1507
> +
> +#define PMC_MSG_DELAY_MIN_US		50
> +#define RESPONSE_REGISTER_LOOP_MAX	20000
> +
> +#define DELAY_MIN_US		2000
> +#define DELAY_MAX_US		3000
> +#define FIFO_SIZE		4096
> +
> +enum amd_pmc_def {
> +	MSG_TEST = 0x01,
> +	MSG_OS_HINT_PCO,
> +	MSG_OS_HINT_RN,
> +};
> +
> +enum s2d_arg {
> +	S2D_TELEMETRY_SIZE = 0x01,
> +	S2D_PHYS_ADDR_LOW,
> +	S2D_PHYS_ADDR_HIGH,
> +	S2D_NUM_SAMPLES,
> +	S2D_DRAM_SIZE,
> +};
> +
> +struct amd_pmc_stb_v2_data {
> +	size_t size;
> +	u8 data[] __counted_by(size);
> +};
> +
> +struct amd_pmc_bit_map {
> +	const char *name;
> +	u32 bit_mask;
> +};
> +
>   struct amd_pmc_dev {
>   	void __iomem *regbase;
>   	void __iomem *smu_virt_addr;

Re: [PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Ilpo Järvinen]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:

> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
> the current code, mailbox command to get the DRAM size info is sent based
> on the values of dev->major and dev->minor.
> 
> But dev->major and dev->minor will have either junk or zero assigned to
> them until at least once a call to amd_pmc_get_smu_version() is made which
> ideally populates dev->major and dev->minor.
> 
> Add a missing amd_pmc_get_smu_version() call to amd_pmc_get_dram_size().
> 
> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> based on review-ilpo, can be added on top of recent stb changes
> 
>  drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
> index 1424c03c1f03..92adf4523736 100644
> --- a/drivers/platform/x86/amd/pmc/pmc.c
> +++ b/drivers/platform/x86/amd/pmc/pmc.c
> @@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
>  {
>  	int ret;
>  
> +	ret = amd_pmc_get_smu_version(dev);
> +	if (ret)
> +		return ret;
> +
>  	switch (dev->cpu_id) {
>  	case AMD_CPU_ID_YC:
>  		if (!(dev->major > 90 || (dev->major == 90 && dev->minor > 39))) {

Does this really belong here? Correct me if I recall wrong but the reason 
why amd_pmc_get_smu_version() was not always done during init was that it 
added noticeable delay to boot? Based on that, I kind of assumed it's 
generic thing (and a such, unrelated to amd_pmc_get_dram_size()) so why is 
this call this deep in the call chain? Perhaps amd_pmc_s2d_init() would 
be more appropriate place for it since you now make the call 
unconditional anyway for that code path?

-- 
 i.

Re: [PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Shyam Sundar S K]

On 10/9/2023 8:25 PM, Mario Limonciello wrote:
> On 10/9/2023 09:47, Shyam Sundar S K wrote:
>> amd_pmc_get_dram_size() is used to get the DRAM size information.
>> But in
>> the current code, mailbox command to get the DRAM size info is sent
>> based
>> on the values of dev->major and dev->minor.
>>
>> But dev->major and dev->minor will have either junk or zero assigned to
>> them until at least once a call to amd_pmc_get_smu_version() is made
>> which
>> ideally populates dev->major and dev->minor.
>>
>> Add a missing amd_pmc_get_smu_version() call to
>> amd_pmc_get_dram_size().
>>
>> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from
>> PMFW")
>> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> based on review-ilpo, can be added on top of recent stb changes
> 
> Hmm, so this is actually going to mean that we end up with a round
> trip call to the SMU as part of probe to get the version when STB is
> initialized.
> 
> I think that's going to translate to a longer amd_pmc startup time,
> which could be problematic if a platform leaves STB enabled by default.
> 
> At a minimum I would suggest to only do the version check when it's
> one that needs it (IE AMD_CPU_ID_YC right now).  But otherwise this
> s2d initialization can't happen the first time it's used?

I remember the long boot time problems, but was not that confined to
only CZN based platforms?

Thanks,
Shyam

> 
>>
>>   drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c
>> b/drivers/platform/x86/amd/pmc/pmc.c
>> index 1424c03c1f03..92adf4523736 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct
>> amd_pmc_dev *dev)
>>   {
>>       int ret;
>>   +    ret = amd_pmc_get_smu_version(dev);
>> +    if (ret)
>> +        return ret;
>> +
>>       switch (dev->cpu_id) {
>>       case AMD_CPU_ID_YC:
>>           if (!(dev->major > 90 || (dev->major == 90 && dev->minor >
>> 39))) {
> 

Re: [PATCH 2/2] platform/x86/amd/pmc: Move structures to pmc header

[Ilpo Järvinen]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:

> As we have a separate header for amd_pmc driver, move the common strutures,
> enums, and macros to the header file.
> 
> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---

> +#define RESPONSE_REGISTER_LOOP_MAX	20000

This is borderline, I'd add prefix for this.

> +#define DELAY_MIN_US		2000
> +#define DELAY_MAX_US		3000
> +#define FIFO_SIZE		4096

These look way too generic names when they are now disconnected from the
code using them, not that DELAY_* ever was a good name for anything :-).

> +enum amd_pmc_def {
> +       MSG_TEST = 0x01,
> +       MSG_OS_HINT_PCO,
> +       MSG_OS_HINT_RN,
> +};

I suggest you consider adding a prefix for these too.


-- 
 i.

Re: [PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Shyam Sundar S K]

On 10/9/2023 8:29 PM, Ilpo Järvinen wrote:
> On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> 
>> amd_pmc_get_dram_size() is used to get the DRAM size information. But in
>> the current code, mailbox command to get the DRAM size info is sent based
>> on the values of dev->major and dev->minor.
>>
>> But dev->major and dev->minor will have either junk or zero assigned to
>> them until at least once a call to amd_pmc_get_smu_version() is made which
>> ideally populates dev->major and dev->minor.
>>
>> Add a missing amd_pmc_get_smu_version() call to amd_pmc_get_dram_size().
>>
>> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from PMFW")
>> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> based on review-ilpo, can be added on top of recent stb changes
>>
>>  drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c b/drivers/platform/x86/amd/pmc/pmc.c
>> index 1424c03c1f03..92adf4523736 100644
>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>> @@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct amd_pmc_dev *dev)
>>  {
>>  	int ret;
>>  
>> +	ret = amd_pmc_get_smu_version(dev);
>> +	if (ret)
>> +		return ret;
>> +
>>  	switch (dev->cpu_id) {
>>  	case AMD_CPU_ID_YC:
>>  		if (!(dev->major > 90 || (dev->major == 90 && dev->minor > 39))) {
> 
> Does this really belong here? Correct me if I recall wrong but the reason 
> why amd_pmc_get_smu_version() was not always done during init was that it 
> added noticeable delay to boot? Based on that, I kind of assumed it's 
> generic thing (and a such, unrelated to amd_pmc_get_dram_size()) so why is 
> this call this deep in the call chain? Perhaps amd_pmc_s2d_init() would 
> be more appropriate place for it since you now make the call 
> unconditional anyway for that code path?
> 

Yes you are right. I see a remark from Mario also on the similar
lines. The best thing is, let me go back to our FW team to understand
when do they report dram_size and what is the implication of calling
get_smu_version().

Thanks,
Shyam

Re: [PATCH 1/2] platform/x86/amd/pmc: Fix fetching DRAM size mechanism

[Mario Limonciello]

On 10/9/2023 10:39, Shyam Sundar S K wrote:
> 
> 
> On 10/9/2023 8:25 PM, Mario Limonciello wrote:
>> On 10/9/2023 09:47, Shyam Sundar S K wrote:
>>> amd_pmc_get_dram_size() is used to get the DRAM size information.
>>> But in
>>> the current code, mailbox command to get the DRAM size info is sent
>>> based
>>> on the values of dev->major and dev->minor.
>>>
>>> But dev->major and dev->minor will have either junk or zero assigned to
>>> them until at least once a call to amd_pmc_get_smu_version() is made
>>> which
>>> ideally populates dev->major and dev->minor.
>>>
>>> Add a missing amd_pmc_get_smu_version() call to
>>> amd_pmc_get_dram_size().
>>>
>>> Fixes: be8325fb3d8c ("platform/x86/amd: pmc: Get STB DRAM size from
>>> PMFW")
>>> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>> ---
>>> based on review-ilpo, can be added on top of recent stb changes
>>
>> Hmm, so this is actually going to mean that we end up with a round
>> trip call to the SMU as part of probe to get the version when STB is
>> initialized.
>>
>> I think that's going to translate to a longer amd_pmc startup time,
>> which could be problematic if a platform leaves STB enabled by default.
>>
>> At a minimum I would suggest to only do the version check when it's
>> one that needs it (IE AMD_CPU_ID_YC right now).  But otherwise this
>> s2d initialization can't happen the first time it's used?
> 
> I remember the long boot time problems, but was not that confined to
> only CZN based platforms?
> 

They're reported originally on CZN platforms.  If I was to hypothesize 
the root cause it is probably that the SMU is busy servicing other 
things during kernel's boot up.

If that's the case then there is no reason to believe it's fixed in 
follow on platforms.

> Thanks,
> Shyam
> 
>>
>>>
>>>    drivers/platform/x86/amd/pmc/pmc.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/amd/pmc/pmc.c
>>> b/drivers/platform/x86/amd/pmc/pmc.c
>>> index 1424c03c1f03..92adf4523736 100644
>>> --- a/drivers/platform/x86/amd/pmc/pmc.c
>>> +++ b/drivers/platform/x86/amd/pmc/pmc.c
>>> @@ -981,6 +981,10 @@ static int amd_pmc_get_dram_size(struct
>>> amd_pmc_dev *dev)
>>>    {
>>>        int ret;
>>>    +    ret = amd_pmc_get_smu_version(dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>>        switch (dev->cpu_id) {
>>>        case AMD_CPU_ID_YC:
>>>            if (!(dev->major > 90 || (dev->major == 90 && dev->minor >
>>> 39))) {
>>

Re: [PATCH 2/2] platform/x86/amd/pmc: Move structures to pmc header

[Shyam Sundar S K]

On 10/9/2023 9:09 PM, Ilpo Järvinen wrote:
> On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> 
>> As we have a separate header for amd_pmc driver, move the common strutures,
>> enums, and macros to the header file.
>>
>> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
> 
>> +#define RESPONSE_REGISTER_LOOP_MAX	20000
> 
> This is borderline, I'd add prefix for this.
> 
>> +#define DELAY_MIN_US		2000
>> +#define DELAY_MAX_US		3000
>> +#define FIFO_SIZE		4096
> 
> These look way too generic names when they are now disconnected from the
> code using them, not that DELAY_* ever was a good name for anything :-).
> 
>> +enum amd_pmc_def {
>> +       MSG_TEST = 0x01,
>> +       MSG_OS_HINT_PCO,
>> +       MSG_OS_HINT_RN,
>> +};
> 
> I suggest you consider adding a prefix for these too.

But these are there from long time, probably from the inception of
this driver :-)

This patch is just cleaning up of the code and move to header for
readability purpose.

You mean to say, make the prefix change while I move the macros to header?

Thanks,
Shyam

> 
> 

Re: [PATCH 2/2] platform/x86/amd/pmc: Move structures to pmc header

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1298 bytes --]

On Mon, 9 Oct 2023, Shyam Sundar S K wrote:

> 
> 
> On 10/9/2023 9:09 PM, Ilpo Järvinen wrote:
> > On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> > 
> >> As we have a separate header for amd_pmc driver, move the common strutures,
> >> enums, and macros to the header file.
> >>
> >> Suggested-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> > 
> >> +#define RESPONSE_REGISTER_LOOP_MAX	20000
> > 
> > This is borderline, I'd add prefix for this.
> > 
> >> +#define DELAY_MIN_US		2000
> >> +#define DELAY_MAX_US		3000
> >> +#define FIFO_SIZE		4096
> > 
> > These look way too generic names when they are now disconnected from the
> > code using them, not that DELAY_* ever was a good name for anything :-).
> > 
> >> +enum amd_pmc_def {
> >> +       MSG_TEST = 0x01,
> >> +       MSG_OS_HINT_PCO,
> >> +       MSG_OS_HINT_RN,
> >> +};
> > 
> > I suggest you consider adding a prefix for these too.
> 
> But these are there from long time, probably from the inception of
> this driver :-)
> 
> This patch is just cleaning up of the code and move to header for
> readability purpose.
> 
> You mean to say, make the prefix change while I move the macros to header?

Make another patch out of it in the same series please.

-- 
 i.