[PATCH] i2c: piix4: Print FCH::PM::S5_RESET_STATUS

[PATCH] i2c: piix4: Print FCH::PM::S5_RESET_STATUS

[Yazen Ghannam]

The following register contains bits that indicate the cause for the
previous reset.

        PMx000000C0 (FCH::PM::S5_RESET_STATUS)

This is helpful for debug, etc., and it only needs to be read once from
a single FCH within the system. The register definition is AMD-specific.

Print it when the FCH MMIO space is first mapped. This register is not
related to I2C functionality, but read it here to leverage the existing
mapping.

Use an "info" log level so that it is printed every boot without requiring
the user to enable debug messages. This is beneficial when debugging
issues that cause spontaneous reboots and are hard to reproduce.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/i2c/busses/i2c-piix4.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 809fbd014cd6..043b29f1e33c 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -100,6 +100,7 @@
 
 #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
 #define SB800_PIIX4_FCH_PM_SIZE			8
+#define SB800_PIIX4_FCH_PM_S5_RESET_STATUS	0xC0
 
 /* insmod parameters */
 
@@ -200,6 +201,9 @@ static int piix4_sb800_region_request(struct device *dev,
 
 		mmio_cfg->addr = addr;
 
+		addr += SB800_PIIX4_FCH_PM_S5_RESET_STATUS;
+		pr_info_once("S5_RESET_STATUS = 0x%08x", ioread32(addr));
+
 		return 0;
 	}
 
-- 
2.34.1

Re: [PATCH] i2c: piix4: Print FCH::PM::S5_RESET_STATUS

[Jean Delvare]

Hi Yazen,

On Fri, 07 Apr 2023 15:37:20 -0500, Yazen Ghannam wrote:
> The following register contains bits that indicate the cause for the
> previous reset.
> 
>         PMx000000C0 (FCH::PM::S5_RESET_STATUS)
> 
> This is helpful for debug, etc., and it only needs to be read once from
> a single FCH within the system. The register definition is AMD-specific.
> 
> Print it when the FCH MMIO space is first mapped. This register is not
> related to I2C functionality, but read it here to leverage the existing
> mapping.
> 
> Use an "info" log level so that it is printed every boot without requiring
> the user to enable debug messages. This is beneficial when debugging
> issues that cause spontaneous reboots and are hard to reproduce.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 809fbd014cd6..043b29f1e33c 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -100,6 +100,7 @@
>  
>  #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
>  #define SB800_PIIX4_FCH_PM_SIZE			8
> +#define SB800_PIIX4_FCH_PM_S5_RESET_STATUS	0xC0
>  
>  /* insmod parameters */
>  
> @@ -200,6 +201,9 @@ static int piix4_sb800_region_request(struct device *dev,
>  
>  		mmio_cfg->addr = addr;
>  
> +		addr += SB800_PIIX4_FCH_PM_S5_RESET_STATUS;
> +		pr_info_once("S5_RESET_STATUS = 0x%08x", ioread32(addr));
> +
>  		return 0;
>  	}
>  

I'm skeptical. For one thing, the register you read is outside of the
mapped MMIO area. SB800_PIIX4_FCH_PM_SIZE is 8 which is less than 0xC0.

For another, printing an hexadecimal value which is AMD-specific is not
going to be really helpful in practice. Is there public documentation
available to decode the value?

Lastly, I can't see why this should happen in
piix4_sb800_region_request() which is going to called repeatedly at
runtime, rather than in piix4_setup_sb800_smba() which is only called
once when the driver is loaded. If this goes in the i2c-piix4 driver at
all... sp5100_tco might be more suitable as that driver is at least
somewhat related to system reset.

Looks like a hack really, and while I understand it is cheap, it would
seem cleaner to put that code in its own platform/x86 driver. Or
arch/x86/kernel/quirks.c maybe.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: piix4: Print FCH::PM::S5_RESET_STATUS

[Yazen Ghannam]

On 4/12/23 12:53, Jean Delvare wrote:
> Hi Yazen,
> 
> On Fri, 07 Apr 2023 15:37:20 -0500, Yazen Ghannam wrote:
>> The following register contains bits that indicate the cause for the
>> previous reset.
>>
>>         PMx000000C0 (FCH::PM::S5_RESET_STATUS)
>>
>> This is helpful for debug, etc., and it only needs to be read once from
>> a single FCH within the system. The register definition is AMD-specific.
>>
>> Print it when the FCH MMIO space is first mapped. This register is not
>> related to I2C functionality, but read it here to leverage the existing
>> mapping.
>>
>> Use an "info" log level so that it is printed every boot without requiring
>> the user to enable debug messages. This is beneficial when debugging
>> issues that cause spontaneous reboots and are hard to reproduce.
>>
>> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
>> ---
>>  drivers/i2c/busses/i2c-piix4.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 809fbd014cd6..043b29f1e33c 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -100,6 +100,7 @@
>>  
>>  #define SB800_PIIX4_FCH_PM_ADDR			0xFED80300
>>  #define SB800_PIIX4_FCH_PM_SIZE			8
>> +#define SB800_PIIX4_FCH_PM_S5_RESET_STATUS	0xC0
>>  
>>  /* insmod parameters */
>>  
>> @@ -200,6 +201,9 @@ static int piix4_sb800_region_request(struct device *dev,
>>  
>>  		mmio_cfg->addr = addr;
>>  
>> +		addr += SB800_PIIX4_FCH_PM_S5_RESET_STATUS;
>> +		pr_info_once("S5_RESET_STATUS = 0x%08x", ioread32(addr));
>> +
>>  		return 0;
>>  	}
>>  
> 
> I'm skeptical. For one thing, the register you read is outside of the
> mapped MMIO area. SB800_PIIX4_FCH_PM_SIZE is 8 which is less than 0xC0.
> 

Hi Jean,

Ah sorry, I overlooked that.

> For another, printing an hexadecimal value which is AMD-specific is not
> going to be really helpful in practice. Is there public documentation
> available to decode the value?
> 

Yes, this register is listed in public documentation. But I expect this value
is only helpful to hardware debug folks. The intent is to have this
information available without requiring any system changes by the user.

> Lastly, I can't see why this should happen in
> piix4_sb800_region_request() which is going to called repeatedly at
> runtime, rather than in piix4_setup_sb800_smba() which is only called
> once when the driver is loaded. If this goes in the i2c-piix4 driver at
> all... sp5100_tco might be more suitable as that driver is at least
> somewhat related to system reset.
> 
> Looks like a hack really, and while I understand it is cheap, it would
> seem cleaner to put that code in its own platform/x86 driver. Or
> arch/x86/kernel/quirks.c maybe.
> 

Yes, that's fair. I'll check out these other places and see if there's a
better fit.

Thank you!

-Yazen