Received-SPF: pass (google.com: domain of linux-kernel+bounces-179812-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <269884a8-da86-4cb5-b4eb-2827578608d4@amd.com>
Date: Wed, 15 May 2024 17:00:57 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] sbrmi: Add support for APML protocols
To: Lee Jones <lee@kernel.org>, Guenter Roeck <linux@roeck-us.net>
Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>, linux-hwmon@vger.kernel.org,
 linux-kernel@vger.kernel.org, Akshay Gupta <Akshay.Gupta@amd.com>,
 arnd@arndb.de, gregkh@linuxfoundation.org
References: <20240502220554.2768611-1-nchatrad@amd.com>
 <20240502220554.2768611-2-nchatrad@amd.com>
 <dd4ef0b6-8272-40b6-9a50-edbeec14d5e4@roeck-us.net>
 <8cf0d519-ae6c-4ed0-899e-899f67479052@amd.com>
 <488e8eed-e0f0-4055-b43c-5422b6302632@roeck-us.net>
 <20240515103217.GA6035@google.com>
Content-Language: en-US
From: "Chatradhi, Naveen Krishna" <naveenkrishna.chatradhi@amd.com>
In-Reply-To: <20240515103217.GA6035@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?TTZNbEJmaWJ6bjZLMjJKVUplcmhFVU5xUlVvb1htYnRKTUJYTFRXYWdaWU01?=
 =?utf-8?B?Yi9iQXlkem9PZ2ZJRExKdVMrL01XZ1ByWWlucWxtVnBZa29zOEVMS2tLOHlO?=
 =?utf-8?B?M1B3RFNQZmtlS1ZRZ3FVZEZyNi9sdm9ucFJYRnZ0b1ZHUUJxMTBQNHM2YzA4?=
 =?utf-8?B?b1NiOG1VSzNNbnlDdWUwbW5IZllTZVEwTDFaQjYzQW9TbVhjOENCeTUzazVV?=
 =?utf-8?B?ZXVBVWd2N0lHaTJjK3pJQXJxb1NTU2Z3OEEyVjJhYmNvR0NMQ1lsOEM5L2Nz?=
 =?utf-8?B?ZUpIZ0VOUnlzT3lwUTdoQ0pRQXZTRnBubGFuRENiejdBRWtwWEZRcEF6OGJt?=
 =?utf-8?B?TkNBcm9pRUwzN0lVUUxYUTVMSVNvL2xzYzdkZi9sV3QxVVl5SStrT3gyWlRo?=
 =?utf-8?B?UTNKMks0WjkydC82ZmtxRGwyQlRYZFN2NURwaU05eHU2VWN0ekhPK1BCTk9v?=
 =?utf-8?B?R3RpblBJUGJ2RG5YaDNLWnRoY1ZUSGhCbzNkeUdQb0JISTZlYW9ScVhTQWVl?=
 =?utf-8?B?MUJqQ3ppeUZGUHdkcWhIOHd6eU5uQ0NTWm5jc3VkMW1GeDZiczV5VkkrS1VN?=
 =?utf-8?B?Z2syREgxZTBrSzM0MFkvVDlSdEpKRTFBdTRFUkVIaUtzVmhmVHpUbmJnSEw4?=
 =?utf-8?B?RGhKVjFjdXZvOThqOHJhSWhland6aHdva2pmU0xmbGVWZk50WDZlV25zdVRn?=
 =?utf-8?B?WGd3ZU84eDN6L0plTnptNG1CaEhDNkNyVXQyRzJGZkZCNXI1ZE05NUd4OXdw?=
 =?utf-8?B?NHRDQVAxckJZOGZROWJtSDJCaXdrelVscmFiK0FPdmJHRUtTd3YxUG0xZVBL?=
 =?utf-8?B?ZkF5RlB5YktCd213bDljMHI4QnUyT2JZKzhXRkxMUzRudW5Dc3hZRWZTODlX?=
 =?utf-8?B?NGUzMzc2dTBOaSt5MTZOU0NIRDJwWjBwR0ZYc3NZa3dLRWc1QWNveERzempx?=
 =?utf-8?B?Y2JDUGJzL0o1UmFRT1lTS01mMEhWaTYwT1VjSVB5bzl3Vk1RVFUzYUlpUGFO?=
 =?utf-8?B?c2cyQm5oNnZNdjhWMTQ2Vk15RFUxK0c3NUhaczVPbkZaMm44R2x0OXhiZkFB?=
 =?utf-8?B?UXdYeFhuTStxWFFFTHZkRnNicUZWSFYvSitGS3Y3WDU3NFNDYml3M24vZG9G?=
 =?utf-8?B?eWNCY0wxbk5kTmNFRXN6QXBiWUh0aTNEOTAySElqbStvaHZpdmdwOFJMMzNw?=
 =?utf-8?B?UnF2NVlveElYVnRRR1Z6a0N1d1VjY0FmRU1Pdjh4WFYxRDh2aTB0elc5NC9Y?=
 =?utf-8?B?WVZuY2QzaFB5ZHR4M1BhcmRjanFVdHM5ZXV4YWR2Q05ERVVrYVZxMkFWTHVY?=
 =?utf-8?B?TW41UGFDciswWEg0bzZuSU91bmtqby9QNEN4eWV4RGgxaExDcGdBUkxrTk0y?=
 =?utf-8?B?bFJVYlRXNDdLMnNVTkdNbjgrUTJPMXdkL3kwYnBBSkRyZ1drMkpDWmx1S3dy?=
 =?utf-8?B?amk4aVZyaVdmMURzTHRzY01WTUFoYVZuMXNjREt2REZONml3L0FpdkJ0Wkla?=
 =?utf-8?B?Nyt4YldSZHAzOXRVU2wxZk5DY3pyNVk0M3JsTmM0VURxVkpoSGNleFZHKzBw?=
 =?utf-8?B?dFVXdVFEdXZGWVVaUzdEeTBIZWZXdko1UWt6OVRIZTZWbFh4Y09TRmJNWmN4?=
 =?utf-8?B?RERhdGFkSlJ1Zk5JT09INEtkazFlMlIweEhrZitiL3lNRk5PWktjM2R4TlFS?=
 =?utf-8?B?WWVrTVhXNW9mcVZtWElqYzNNeitQYU5lT2xjMVBweiszd2R6K0Z5bU5lS2Rk?=
 =?utf-8?B?bkNNc29RMkYvTVc4dlpTTEhlVERqWlkwZlZuOWlYWllKVHE5SDQ1WTBaMWI0?=
 =?utf-8?B?Y2JibW5Hd2lueDd3TCs2MUhtdUxQRDR2WHp5cExHNmJwUDF4enp2OGpXbzdy?=
 =?utf-8?B?SHF5ZzFZRzROQW5ZRUU1aHFmSHpUSTBQZ081b2FCaTNTcWtad2QzS21EMEFO?=
 =?utf-8?B?YU5TM1c2S3NMYTlDNGtTekpQZXBKZEZVVnVnaFcxdFg4bWY4dUZNTytkT2x5?=
 =?utf-8?B?Tm9MSHV1MW1jenZsUUNMdjFWZ1JUSkpJcGs3RU1NY3BzR3dwYXdrSWFxTE1x?=
 =?utf-8?B?WmxNZkVkamdGd1NOWS9MWkc4UkZIN01HNWt3dVRpaU1kcWQ5NVcxdVo4cmRP?=
 =?utf-8?Q?UW6Cs/tjxwl1+Bwe0DJgdj1j2?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c006c197-3464-40f0-5dbf-08dc74d281e2
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5286.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2024 11:31:05.4094
 (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: 78CBtMeduirvqKyQXfvDC551jEYzKooJ9PmGBDprndTx0qUpFB/UsfdECQXq2YGrAemx4EMVDiYWfWGzuLPidg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7571


On 5/15/2024 4:02 PM, Lee Jones wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Tue, 14 May 2024, Guenter Roeck wrote:
>
>> On 5/14/24 12:15, Chatradhi, Naveen Krishna wrote:
>>> + @Misc and @MFD maintainers in CC
>>>
>>> Hi
>>>
>>> On 5/3/2024 3:56 AM, Guenter Roeck wrote:
>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>
>>>>
>>>> On 5/2/24 15:05, Naveen Krishna Chatradhi wrote:
>>>>> From: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>>>>>
>>>>> The present sbrmi module only support reporting power. However, AMD data
>>>>> center processors support various system management functionality
>>>>> Out-of-band over Advanced Platform Management Link APML.
>>>>>
>>>>> Register a miscdevice, which creates a device /dev/sbrmiX with an IOCTL
>>>>> interface for the user space to invoke the following protocols.
>>>>>     - Mailbox read/write (already defined in sbrmi_mailbox_xfer())
>>>>>     - CPUID read
>>>>>     - MCAMSR read
>>>>>
>>>> This is not hardware monitoring functionality and would have to reside
>>>> elsewhere, outside the hwmon subsystem.
>>> I thought as much, please provide your opinion on the following approach.
>>>
>>> Background: Present sbrmi under hwmon subsystem is probed as an i2c driver and reports power.
>>>
>>> However, APML interface defines few other protocols to support OOB system management functionality.
>>>
>>> As adding the core functionality of the APML interface in drivers/hwmon/sbrmi is not the correct approach.
>>>
>>> We would like do the following
>>>
>>> 1. Move the i2c client probe, misc device registration and rmi_mailbox_xfer() function to a drivers/misc.
>>>
>>> 2. Add an MFD device with a cell, which probes the hwmon/sbrmi and continues to report power using the symbols exported by the misc/sbrmi.
>>>
>> afaik mfd API function must not be used outside drivers/mfd. The mfd maintainer
>> is (or at least used to be) pretty strict on that. The core code of a
>> multi-function device might better be implemented in drivers/mfd, with
>> drivers in drivers/misc (for the misc device) and drivers/hwmon/ (for
>> hwmon functionality). The misc and hwmon drivers could then communicate
>> with the core using regmap.

Thanks Guenter, for the inputs.

> Yes, please only use the MFD API from drivers/mfd.
>
> There are 'offenders' that slipped by me, but in general if you need to
> create an MFD then it should be located in the MFD subsystem.
Thanks Lee, how about

1. Move the i2c client probe, misc device registration and rmi_mailbox_xfer() function from existing hwmon/sbrmi.c to a drivers/mfd (Instead of drivers/misc)

    a. Provide an ioctl interface through misc device node /dev/sbrmiX for the user space to invoke the following protocols.
      - Mailbox xfer (already defined in sbrmi_mailbox_xfer())
      - CPUID access
      - MCAMSR access

2. Add an MFD device with a cell, which probes the hwmon/sbrmi and continues to report power using the symbols exported by the mfd/sbrmi.

>
>> drivers/mailbox/ supports mailbox style communication. I don't know if that
>> would apply to the mailbox functionality implemented here.
I've explored that path, and APML mailbox does not fit well into the 
design, we do not have a dedicated interrupt line as well.
> --
> Lee Jones [李琼斯]

regards,

Naveenk
>