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;
Message-ID: <e41399a4-c6d7-2b79-3f87-1a0efa64dffa@amd.com>
Date:   Wed, 12 Apr 2023 13:13:49 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Cc:     yazen.ghannam@amd.com, 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
Content-Language: en-US
To:     Jean Delvare <jdelvare@suse.de>
References: <20230407203720.18184-1-yazen.ghannam@amd.com>
 <20230412185358.4d6427ce@endymion.delvare>
From:   Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <20230412185358.4d6427ce@endymion.delvare>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aXJPL2E2aWFMSENjbVRFS3h2RS9hcG5id0dpZjk1TUhETHJiY1g5V2ZRNFd3?=
 =?utf-8?B?TExRKzlaVVg4VFdsM2Zob0E1Q09ncDdWUzdhaHQyZHBjMkFxVFRHL1NEdG5I?=
 =?utf-8?B?ZjJreEFwcTJxMjJYdVRkSTBvT2JHLzhvNGlMNTFFMWJFY1l1Q0sxNXNDOTNV?=
 =?utf-8?B?bmVWNUQ4OVp2OUJoYjJMKzBmS2pEcnBsRFNxTDVWRmhtc0kzRjJBOFR3a3JE?=
 =?utf-8?B?ZWtxZHpBWERjTVplc0hYekNvQ0tlK3VLSE40VmhNS25xMkNHLzJUaVU3MW9Z?=
 =?utf-8?B?QjlZUEJZS2pqVDgwYWNSQUxoRCtmNlM2TDRMSDR3eUZkN0hKeWdiaWRYMVpB?=
 =?utf-8?B?eE12c2hpTDFUMlZCSEZsK2hXV1paNkpVVTYyT20zWU1IQVZJVzJVNkFYeTZH?=
 =?utf-8?B?TndoV2ZHcldnV3BBNlJMdnZNZmgraUJDSGVSNWNpK0t3cE5DTURmR2dMS0Vz?=
 =?utf-8?B?c3N6L3dUdmlRVXZRSFRlZkFrbDcxOVBxbVk3bmVpOEpIaVNUcGdiV0ZxUDJ0?=
 =?utf-8?B?N0NHRFc5dmtjSE9ieVBwcDF2Q2FUNGxDUFBwd0hPZEdjNWZ2VlRlTG5yZWhD?=
 =?utf-8?B?Smg0TlBKZUwwSFREL1RVbVFIZHlXQjMvY2RTbUNlWFh1enRPZ0ZwRTMxQmVy?=
 =?utf-8?B?OVFsSnkwYVdGaFBJamhjQ2ttNFR1KzNqYnBHbVByTGp6R1IrMFlIaDRIMHBp?=
 =?utf-8?B?N2c1OWRzMmlhNXkyU2lSRDByRUx0L1hvN2pZeDhCb0hYaGViOWQ5K1ZpT0Qy?=
 =?utf-8?B?bTU0emd0VXIxcUNzZCsxMktvdS8wRzdlQy9JY0tiL2liQmpEWlpkclBpUW9y?=
 =?utf-8?B?aEZkWkJvWklrb0RkQzV5Wm9RMktxVkVMUEk3YXNobjUwcW50c0VSVVhQMEVp?=
 =?utf-8?B?bnU0ZVBFNXp4VHVFVVVmZktYYVRNMDAwNUlSYUZBNWhuWHN5bjY0cE1sY2tU?=
 =?utf-8?B?WDFaQ0NvOUlZTzNpN1RQL1ZRL3k2TWJCeFlvY3VrNDE0U0F1VkZhTEFqNFZn?=
 =?utf-8?B?YUp1SGFZaU9yVjdOeU9oM29UY2huOTZ4SFZEQmNZR1U4alR2STR4OXp4Q1Vh?=
 =?utf-8?B?b0ZIcDhDYVk4bWlZUVZzL1pUblFkNUhIR2lTbThlZGlGSDF1YzRXT3Z3S3p0?=
 =?utf-8?B?RE1JQUZCWVJmK1ZVYjhBa1lYbVdibHdpaTVjU3llMVF0S2l3TnF0VWFRY0tT?=
 =?utf-8?B?Vm1yeDlIK3d6MmM0NjJMdjV1bkJwMTYvNVROQTdPSjVDbUF2Y0RrU2RVL3BB?=
 =?utf-8?B?QklGQXp6d1UyWi9ZMnJaYzdSbVE1elNSK3VDZWpKRFFTc0w4UUJpSTZrUmlH?=
 =?utf-8?B?M2svbFIxNGRDWFRHYWpvcmR3UE51QjBVMFNOczZVa2w5ZWVYS1JHS2dTVjhF?=
 =?utf-8?B?akxyRkRHeURONVYyMWVmT0l4RE1GLytpb210ekl3QUdIZmU0V2JGbFY3UFAy?=
 =?utf-8?B?ZitiRUs4OXc3VkRCVS9ia2lGU1JIanVtYTNGSG0vajVGcG9BN0FHbFhQR0xU?=
 =?utf-8?B?UzlTSnJEc3RqQXFZMEI1dmk5Q0lBQ2s0clMyQVM5dXV1dXBrd2t4blNFdG5B?=
 =?utf-8?B?WXlXVi9oNEh2dDg1aTZWRjVTMTJjQjlOVklRL2w2aE1uazcrRW1xSUZLYVNk?=
 =?utf-8?B?UE5oT1krQW5nY3kvV21KVDBCNE0vcFR3eDJ1cGF5ODlqRURURXZKQkhUN0V0?=
 =?utf-8?B?MmFwaEg4cmNtcm90bnZnd0Rtb2o0VDBjdk1WcTcvVkNjNzlWU285Zmx2b3JJ?=
 =?utf-8?B?TGh1OERvdzJnZE55anY5bnZRRnI3by9yaUVqOEg0NFhZNFJyNCt0emxrazlF?=
 =?utf-8?B?QUd0SkhGUERPS2NweFF3cDVpL1VobXhJbHR1ZVBCcnlqcUpGNkt1Y2ZBZkNs?=
 =?utf-8?B?WHF2VFRBRGM0TVp2TndLaE8yZXhSVEtZYnNqRTBPQmVXdm5qeVV5dHRnODlI?=
 =?utf-8?B?MlhlVlZ3Uk5hTFI3NzArdDJ0UnF1UGtRQ1o1UUZ1R0lGaWFSM1JCZTk1UzA4?=
 =?utf-8?B?OFpONTEweVVCdy92S0c3LzBoY0E5UFVPNVhUeEdxRnE3MS8wUklVREI3ZWRY?=
 =?utf-8?B?RjNJRjRrT0x3d0pvVDdSVmQ3azhuTUlKVTZTeDlQckQ3MVg3d2xMaWpxbDhR?=
 =?utf-8?Q?cxXOhPfsqfxYbWOpK4+t3tm2k?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 083d9bc8-1431-4fa2-fd28-08db3b7949a9
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2023 17:13:51.7770
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: fkBRvJXF3LwSCp4CPzBLJy8R52XbsFlWB2nHPf9KzeM2wKa9tWGhmH7Eu1UbwTiCKz+AlCZxFQrzYJrPrSWYEA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR12MB4410
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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