diff mbox series

[v2,3/8] platform/x86/amd/pmc: Define enum for S2D/PMC msg_port

Message ID 20241029155440.3499273-4-Shyam-sundar.S-k@amd.com (mailing list archive)
State Changes Requested, archived
Headers show
Series platform/x86/amd/pmc: Updates to AMD PMC driver | expand

Commit Message

Shyam Sundar S K Oct. 29, 2024, 3:54 p.m. UTC 
To distinguish between the PMC message port and the S2D (Spill to DRAM)
message port, replace the use of 0 and 1 with an enum.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Ilpo Järvinen Nov. 1, 2024, 10:28 a.m. UTC | #1 
On Tue, 29 Oct 2024, Shyam Sundar S K wrote:

> To distinguish between the PMC message port and the S2D (Spill to DRAM)
> message port, replace the use of 0 and 1 with an enum.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> index 5efec020ecac..2b06861c479b 100644
> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> @@ -47,6 +47,11 @@ enum s2d_arg {
>  	S2D_DRAM_SIZE,
>  };
>  
> +enum s2d_msg_port {
> +	MSG_PORT_PMC,
> +	MSG_PORT_S2D,
> +};
> +
>  struct amd_stb_v2_data {
>  	size_t size;
>  	u8 data[] __counted_by(size);
> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>  
>  	/* Spill to DRAM num_samples uses separate SMU message port */
> -	dev->msg_port = 1;
> +	dev->msg_port = MSG_PORT_S2D;
>  
>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>  	if (ret)
> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>  	/* Get the num_samples to calculate the last push location */
>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>  	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> +	dev->msg_port = MSG_PORT_PMC;
>  	if (ret) {
>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>  		return ret;
> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>  				    &amd_stb_debugfs_fops);
>  
>  	/* Spill to DRAM feature uses separate SMU message port */
> -	dev->msg_port = 1;
> +	dev->msg_port = MSG_PORT_S2D;
>  
>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>  	if (size != S2D_TELEMETRY_BYTES_MAX)
> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>  
>  	/* Clear msg_port for other SMU operation */
> -	dev->msg_port = 0;
> +	dev->msg_port = MSG_PORT_PMC;
>  
>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>  	if (!dev->stb_virt_addr)

This change is incomplete, you missed all places using it:

if (dev->msg_port) { 

+ add helper for this:

dev->msg_port ? "S2D" : "PMC"
Shyam Sundar S K Nov. 5, 2024, 5:04 a.m. UTC | #2 
On 11/1/2024 15:58, Ilpo Järvinen wrote:
> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> 
>> To distinguish between the PMC message port and the S2D (Spill to DRAM)
>> message port, replace the use of 0 and 1 with an enum.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
>> index 5efec020ecac..2b06861c479b 100644
>> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
>> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
>> @@ -47,6 +47,11 @@ enum s2d_arg {
>>  	S2D_DRAM_SIZE,
>>  };
>>  
>> +enum s2d_msg_port {
>> +	MSG_PORT_PMC,
>> +	MSG_PORT_S2D,
>> +};
>> +
>>  struct amd_stb_v2_data {
>>  	size_t size;
>>  	u8 data[] __counted_by(size);
>> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>>  
>>  	/* Spill to DRAM num_samples uses separate SMU message port */
>> -	dev->msg_port = 1;
>> +	dev->msg_port = MSG_PORT_S2D;
>>  
>>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>>  	if (ret)
>> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>  	/* Get the num_samples to calculate the last push location */
>>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>>  	/* Clear msg_port for other SMU operation */
>> -	dev->msg_port = 0;
>> +	dev->msg_port = MSG_PORT_PMC;
>>  	if (ret) {
>>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>>  		return ret;
>> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>  				    &amd_stb_debugfs_fops);
>>  
>>  	/* Spill to DRAM feature uses separate SMU message port */
>> -	dev->msg_port = 1;
>> +	dev->msg_port = MSG_PORT_S2D;
>>  
>>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>>  
>>  	/* Clear msg_port for other SMU operation */
>> -	dev->msg_port = 0;
>> +	dev->msg_port = MSG_PORT_PMC;
>>  
>>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>>  	if (!dev->stb_virt_addr)
> 
> This change is incomplete, you missed all places using it:
> 
> if (dev->msg_port) { 
> 
> + add helper for this:
> 
> dev->msg_port ? "S2D" : "PMC"
> 


I am not sure if I understand your comment fully. Can you please
elaborate?

Thanks,
Shyam
Ilpo Järvinen Nov. 5, 2024, 9:44 a.m. UTC | #3 
On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
> On 11/1/2024 15:58, Ilpo Järvinen wrote:
> > On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
> > 
> >> To distinguish between the PMC message port and the S2D (Spill to DRAM)
> >> message port, replace the use of 0 and 1 with an enum.
> >>
> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> >> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
> >> index 5efec020ecac..2b06861c479b 100644
> >> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
> >> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
> >> @@ -47,6 +47,11 @@ enum s2d_arg {
> >>  	S2D_DRAM_SIZE,
> >>  };
> >>  
> >> +enum s2d_msg_port {
> >> +	MSG_PORT_PMC,
> >> +	MSG_PORT_S2D,
> >> +};
> >> +
> >>  struct amd_stb_v2_data {
> >>  	size_t size;
> >>  	u8 data[] __counted_by(size);
> >> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> >>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
> >>  
> >>  	/* Spill to DRAM num_samples uses separate SMU message port */
> >> -	dev->msg_port = 1;
> >> +	dev->msg_port = MSG_PORT_S2D;
> >>  
> >>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
> >>  	if (ret)
> >> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
> >>  	/* Get the num_samples to calculate the last push location */
> >>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
> >>  	/* Clear msg_port for other SMU operation */
> >> -	dev->msg_port = 0;
> >> +	dev->msg_port = MSG_PORT_PMC;
> >>  	if (ret) {
> >>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
> >>  		return ret;
> >> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
> >>  				    &amd_stb_debugfs_fops);
> >>  
> >>  	/* Spill to DRAM feature uses separate SMU message port */
> >> -	dev->msg_port = 1;
> >> +	dev->msg_port = MSG_PORT_S2D;
> >>  
> >>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
> >>  	if (size != S2D_TELEMETRY_BYTES_MAX)
> >> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
> >>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
> >>  
> >>  	/* Clear msg_port for other SMU operation */
> >> -	dev->msg_port = 0;
> >> +	dev->msg_port = MSG_PORT_PMC;
> >>  
> >>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
> >>  	if (!dev->stb_virt_addr)
> > 
> > This change is incomplete, you missed all places using it:
> > 
> > if (dev->msg_port) { 
> > 
> > + add helper for this:
> > 
> > dev->msg_port ? "S2D" : "PMC"
> > 
> 
> 
> I am not sure if I understand your comment fully. Can you please
> elaborate?

There are users of dev->msg_port that should be also touched by this 
change but weren't.

For the printing, I suggested a helper function which returns the correct 
string so you don't need to do the compare within print argument.
Shyam Sundar S K Nov. 5, 2024, 5:39 p.m. UTC | #4 
On 11/5/2024 15:14, Ilpo Järvinen wrote:
> On Tue, 5 Nov 2024, Shyam Sundar S K wrote:
>> On 11/1/2024 15:58, Ilpo Järvinen wrote:
>>> On Tue, 29 Oct 2024, Shyam Sundar S K wrote:
>>>
>>>> To distinguish between the PMC message port and the S2D (Spill to DRAM)
>>>> message port, replace the use of 0 and 1 with an enum.
>>>>
>>>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> Co-developed-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>  drivers/platform/x86/amd/pmc/mp1_stb.c | 13 +++++++++----
>>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
>>>> index 5efec020ecac..2b06861c479b 100644
>>>> --- a/drivers/platform/x86/amd/pmc/mp1_stb.c
>>>> +++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
>>>> @@ -47,6 +47,11 @@ enum s2d_arg {
>>>>  	S2D_DRAM_SIZE,
>>>>  };
>>>>  
>>>> +enum s2d_msg_port {
>>>> +	MSG_PORT_PMC,
>>>> +	MSG_PORT_S2D,
>>>> +};
>>>> +
>>>>  struct amd_stb_v2_data {
>>>>  	size_t size;
>>>>  	u8 data[] __counted_by(size);
>>>> @@ -156,7 +161,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>>>  		dev_err(dev->dev, "error writing to STB: %d\n", ret);
>>>>  
>>>>  	/* Spill to DRAM num_samples uses separate SMU message port */
>>>> -	dev->msg_port = 1;
>>>> +	dev->msg_port = MSG_PORT_S2D;
>>>>  
>>>>  	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
>>>>  	if (ret)
>>>> @@ -173,7 +178,7 @@ static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
>>>>  	/* Get the num_samples to calculate the last push location */
>>>>  	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
>>>>  	/* Clear msg_port for other SMU operation */
>>>> -	dev->msg_port = 0;
>>>> +	dev->msg_port = MSG_PORT_PMC;
>>>>  	if (ret) {
>>>>  		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
>>>>  		return ret;
>>>> @@ -266,7 +271,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>>>  				    &amd_stb_debugfs_fops);
>>>>  
>>>>  	/* Spill to DRAM feature uses separate SMU message port */
>>>> -	dev->msg_port = 1;
>>>> +	dev->msg_port = MSG_PORT_S2D;
>>>>  
>>>>  	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
>>>>  	if (size != S2D_TELEMETRY_BYTES_MAX)
>>>> @@ -284,7 +289,7 @@ int amd_s2d_init(struct amd_pmc_dev *dev)
>>>>  	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>>>>  
>>>>  	/* Clear msg_port for other SMU operation */
>>>> -	dev->msg_port = 0;
>>>> +	dev->msg_port = MSG_PORT_PMC;
>>>>  
>>>>  	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
>>>>  	if (!dev->stb_virt_addr)
>>>
>>> This change is incomplete, you missed all places using it:
>>>
>>> if (dev->msg_port) { 
>>>
>>> + add helper for this:
>>>
>>> dev->msg_port ? "S2D" : "PMC"
>>>
>>
>>
>> I am not sure if I understand your comment fully. Can you please
>> elaborate?
> 
> There are users of dev->msg_port that should be also touched by this 
> change but weren't.
> 
> For the printing, I suggested a helper function which returns the correct 
> string so you don't need to do the compare within print argument.
> 

Got it. I have addressed this helper function in 8/13 of v3 just sent
out now. Please have a look at it.

Thanks,
Shyam
diff mbox series

Patch

diff --git a/drivers/platform/x86/amd/pmc/mp1_stb.c b/drivers/platform/x86/amd/pmc/mp1_stb.c
index 5efec020ecac..2b06861c479b 100644
--- a/drivers/platform/x86/amd/pmc/mp1_stb.c
+++ b/drivers/platform/x86/amd/pmc/mp1_stb.c
@@ -47,6 +47,11 @@  enum s2d_arg {
 	S2D_DRAM_SIZE,
 };
 
+enum s2d_msg_port {
+	MSG_PORT_PMC,
+	MSG_PORT_S2D,
+};
+
 struct amd_stb_v2_data {
 	size_t size;
 	u8 data[] __counted_by(size);
@@ -156,7 +161,7 @@  static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 		dev_err(dev->dev, "error writing to STB: %d\n", ret);
 
 	/* Spill to DRAM num_samples uses separate SMU message port */
-	dev->msg_port = 1;
+	dev->msg_port = MSG_PORT_S2D;
 
 	ret = amd_pmc_send_cmd(dev, 0, &val, STB_FORCE_FLUSH_DATA, 1);
 	if (ret)
@@ -173,7 +178,7 @@  static int amd_stb_debugfs_open_v2(struct inode *inode, struct file *filp)
 	/* Get the num_samples to calculate the last push location */
 	ret = amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &num_samples, dev->s2d_msg_id, true);
 	/* Clear msg_port for other SMU operation */
-	dev->msg_port = 0;
+	dev->msg_port = MSG_PORT_PMC;
 	if (ret) {
 		dev_err(dev->dev, "error: S2D_NUM_SAMPLES not supported : %d\n", ret);
 		return ret;
@@ -266,7 +271,7 @@  int amd_s2d_init(struct amd_pmc_dev *dev)
 				    &amd_stb_debugfs_fops);
 
 	/* Spill to DRAM feature uses separate SMU message port */
-	dev->msg_port = 1;
+	dev->msg_port = MSG_PORT_S2D;
 
 	amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size, dev->s2d_msg_id, true);
 	if (size != S2D_TELEMETRY_BYTES_MAX)
@@ -284,7 +289,7 @@  int amd_s2d_init(struct amd_pmc_dev *dev)
 	stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
 
 	/* Clear msg_port for other SMU operation */
-	dev->msg_port = 0;
+	dev->msg_port = MSG_PORT_PMC;
 
 	dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr, dev->dram_size);
 	if (!dev->stb_virt_addr)