Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Apr 2023 18:53:58 +0200
From:   Jean Delvare <jdelvare@suse.de>
To:     Yazen Ghannam <yazen.ghannam@amd.com>
Cc:     <linux-i2c@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <terry.bowman@amd.com>
Subject: Re: [PATCH] i2c: piix4: Print FCH::PM::S5_RESET_STATUS
Message-ID: <20230412185358.4d6427ce@endymion.delvare>
In-Reply-To: <20230407203720.18184-1-yazen.ghannam@amd.com>
References: <20230407203720.18184-1-yazen.ghannam@amd.com>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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