Received-SPF: pass (google.com: domain of linux-kernel+bounces-191647-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <6f4bc109-00d0-47b0-a581-b96a6152545c@amd.com>
Date: Mon, 27 May 2024 21:19:10 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting
To: "Luke D. Jones" <luke@ljones.dev>, hdegoede@redhat.com
Cc: ilpo.jarvinen@linux.intel.com, corentin.chary@gmail.com,
 platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240528013626.14066-1-luke@ljones.dev>
 <20240528013626.14066-9-luke@ljones.dev>
Content-Language: en-US
From: "Limonciello, Mario" <mario.limonciello@amd.com>
In-Reply-To: <20240528013626.14066-9-luke@ljones.dev>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?NVVHV3kyM0tHSE1CTU83SDFGcVVkenRmcUpiQjVNcFYvUWdnWCtycHNnWlpn?=
 =?utf-8?B?VmNhWVYrQlVxclBvcHl5Z2FYZk42SDhKZDZTZWxacWwrUGcrRFkyL0VnN2JS?=
 =?utf-8?B?RlZBRmoxd2ZwVmgwTGdSUEw0dVBTeWFQZFBDYTQ4SE1wWVFqZWd4VjVCR05T?=
 =?utf-8?B?UElMTTFoZm96OHNNbXBidDR3YUxWYm1ScXY2d0daUFBiZlZFZ1JjK0gxZE5C?=
 =?utf-8?B?cjN5Z3JGaEdMajM2amk2SHBxUjliS2RyVnNxME1PaVZyVFZRRjZRSktzUU9W?=
 =?utf-8?B?a3k5Q0VRcXdJYjg3Y3R6UkV4aE5tTUU2MjNSSTNCSG9PQ29RYVRrRjJoWDh2?=
 =?utf-8?B?eUVXcjRhVm1XQzQ0d21DZ2tlSTBUemt0clJJMHNYWC8rK3pPRVJtQk5Ua0tF?=
 =?utf-8?B?MmVhK2hHODZqdnhjYTZvYlZ3aDFVVkk0b3lJUTcrN1JyajRZY1dGVWN1dk1X?=
 =?utf-8?B?dDhEUlZ6Y1V2YlBKd3NYUDNvdldtNzNFeGZkcThVa1NYdXhQYzJRL3dkdVN5?=
 =?utf-8?B?cnZaVEdObEJWa2t5aEFMbHptSDQ3MWZoQTdkN1c0T1dzb0NLTEwrbjVhZlZR?=
 =?utf-8?B?OUp1OXNCNzl1ZUxMNExqaVkyMEZOL2RNVlE5WjM0djdyQSsya09na0NOWHM2?=
 =?utf-8?B?ZUhDaUs2YWhzWk1UMXV5SS9CczlYaEJod2NBaXVTb2s0RUlKTW9BZzNuOWxU?=
 =?utf-8?B?S2FxUW0vSFp1bzMyeXo2SUdvdkVzWHdzclg4UlpzZE5jbnU5ZHlSZDEzZ0hB?=
 =?utf-8?B?QlBYVXhLcmZGaXFHSHBaYnBEcldMbGVOdFZVYytjcGpqWGpuZGE1Z1VTSHFl?=
 =?utf-8?B?WkdPWWNhOE9ha3JrbGFlL1l1ZW9SYU5ndEN3ZjFFMGJ2YUV2SDhBVVlURjMz?=
 =?utf-8?B?UGRXMnRNMXlJM2hhVWNNZzZkTEpNc2FxdWNSR05KUENzeHRDRDJMT3RGelRw?=
 =?utf-8?B?MFUwY3FzUk50VG5qbmwrMURKRnNoaTIrWUh5NmpjQkZCNnhGOWdJbHRCUnlM?=
 =?utf-8?B?VWpoUUFNNVZkS1BNWlpJQmJpUjA1UytaWFVmcUs3S0Yxc0lUVUVGeGJMM2pO?=
 =?utf-8?B?ZFJHWmIzRE90YTJpbFVTbkFRK0NLalBJT3Zvc2UvMFQzamU4ajJHY1d1NTlZ?=
 =?utf-8?B?LzdzUTZGY2JDYkkwNjV6R3QzS25uTjZ3VmtIbXc1Q1kyUFphd2JSb1RJcWRv?=
 =?utf-8?B?WjNtVnp5MzFaZ2pKOFBtRTYwQ2Z6QnVCc2UvL1NWcEFZV2tKZ21OaERQaUUx?=
 =?utf-8?B?S3Z0VVZzRkdmbDdrbDZnQWsyTU5lUHd3MUZUb2NiZmFidDJ3aGc1MzRheXFJ?=
 =?utf-8?B?NXlNdnlVMEdnOXJiQVhHRldLMnlIZWRhZ3dmY09HR1Nlam5RcE9lRGVvN1pW?=
 =?utf-8?B?amJrSThuVnByT0NESmFYNWpPTEw1c2J0YkgvTXJET3ZRUFp3bEsxOVBQV0tY?=
 =?utf-8?B?dzNOUmpmQUdhNXRPU3VnSGdPSzRJK2tUV3M3aGJyVXdnU05nVlk1QjBwR1VN?=
 =?utf-8?B?OGk1dDFINXhPbUlkR3Y0N0ZkQy9JY0xrT0ZtNm55MGFFUDZ4SHNYY3FoaFRR?=
 =?utf-8?B?b0d1Wkw4TUFpcEZBOW82c1p3cm10UWpoRC9mTFo0Ym9STnlBeE1TVk9IdTJL?=
 =?utf-8?B?ZkUzMzlIN1lqOHNhbFF2SlhXcFROZlVhY0NLOFErc0RNZ0cyZ1MzaEl0cDNv?=
 =?utf-8?B?QzBwNGhsNS9kbE1sU2tFWm9kaXJwUVZKNEZpdUhHOTE3WVhwSTg3eE0yREpB?=
 =?utf-8?B?eE01bXN0NEQyL09oMnI5MkJJUUZzZnZzMk1FOGMrVk00U21zdzZnUm4yNUkz?=
 =?utf-8?B?YjJGb3l4UHozQVBTMzAwc0tXcUs4bmlOZWlpS3BMMjNuSVB6WDAxTFY3S2I2?=
 =?utf-8?B?WThlY01VQzVJTkRpVk5XdmxndmI4ckw4S3B2YU5hVEUxUnF6VzhWVXBjeFdS?=
 =?utf-8?B?QzFhWGRPSGlLZERGZmR6ejRMdVR0OENvdGtDenZ2WnJQd2VtZjBzai9RdDht?=
 =?utf-8?B?MEZFZEdVaU1ONFc5L244SHVwL2poUXNRUVJOSnAvSnBXZFJPWndIcVpheVVR?=
 =?utf-8?B?aEZjOEYxcHRyVEdNTTYveEkzNWZ6Q25JR09qYXpYaFFUNmxlMk51NFpvQ0E5?=
 =?utf-8?Q?fm2T6cGRPO8zLVx9Lrqad492j?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a47ff0da-b50b-4dea-3dba-08dc7ebc908e
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 02:19:12.5063
 (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: OZkLUPxKOE4QHl5iohE/mMCUFpUgx1WCpaNzSHgx4d4Htlh8JMEG6Twh0MaotVmRTsqJDugVjY+I73p1EaFNbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB9323


On 5/27/2024 8:36 PM, Luke D. Jones wrote:
> Exposes the APU memory setting available on a few ASUS models such as
> the ROG Ally.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>   .../ABI/testing/sysfs-platform-asus-wmi       |   8 ++
>   drivers/platform/x86/asus-wmi.c               | 109 ++++++++++++++++++
>   include/linux/platform_data/x86/asus-wmi.h    |   3 +
>   3 files changed, 120 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index ac881e72e374..d221a3bc1a81 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -245,3 +245,11 @@ Description:
>   		Show the maximum performance and efficiency core countin format
>   		0x[E][P] where [E] is the efficiency core count, and [P] is
>   		the perfromance core count.
> +
> +What:		/sys/devices/platform/<platform>/apu_mem
> +Date:		Jun 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Set the maximum available system memory for the APU.
> +		  * Min=0, Max=8

What is the unit?  It seems like multiples of something?

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index f62a36dfcd4b..4b5fbae8c563 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled);
>   WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX);
>   static DEVICE_ATTR_RO(cores_max);
>   
> +/* Device memory available to APU */
> +
> +static ssize_t apu_mem_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int err;
> +	u32 mem;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem);
> +	if (err < 0)
> +		return err;
> +
> +	switch (mem) {
> +	case 256:
> +		mem = 0;
> +		break;
> +	case 258:
> +		mem = 1;
> +		break;
> +	case 259:
> +		mem = 2;
> +		break;
> +	case 260:
> +		mem = 3;
> +		break;
> +	case 261:
> +		mem = 4;
> +		break;
> +	case 262:
> +		mem = 8;
> +		break;
> +	case 263:
> +		mem = 5;
> +		break;
> +	case 264:
> +		mem = 6;
> +		break;
> +	case 265:
> +		mem = 7;
> +		break;
> +	default:
> +		mem = 4;
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "%d\n", mem);
> +}
> +
> +static ssize_t apu_mem_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct asus_wmi *asus = dev_get_drvdata(dev);
> +	int result, err;
> +	u32 mem;
> +
> +	result = kstrtou32(buf, 10, &mem);
> +	if (result)
> +		return result;
> +
> +	switch (mem) {
> +	case 0:
> +		mem = 0;
> +		break;
> +	case 1:
> +		mem = 258;
> +		break;
> +	case 2:
> +		mem = 259;
> +		break;
> +	case 3:
> +		mem = 260;
> +		break;
> +	case 4:
> +		mem = 261;
> +		break;
> +	case 5:
> +		mem = 263;
> +		break;
> +	case 6:
> +		mem = 264;
> +		break;
> +	case 7:
> +		mem = 265;
> +		break;
> +	case 8:
> +		mem = 262;

Is case 8 a mistake, or intentionally out of order?

> +		break;
> +	default:
> +		return -EIO;
> +	}
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result);
> +	if (err) {
> +		pr_warn("Failed to set apu_mem: %d\n", err);
> +		return err;
> +	}
> +
> +	pr_info("APU memory changed, reboot required\n");

If you're logging something into the logs for this, I'd say make it more 
useful.

"APU memory changed to %d MB"

> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem");

So this is a case that the BIOS attributes API I mentioned before would 
be REALLY useful.  There is a pending_reboot sysfs file that userspace 
can query to know if a given setting requires a reboot or not.

Fwupd also uses this attribute to know to delay BIOS updates until the 
system has been rebooted.

> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(apu_mem);
> +
>   /* Tablet mode ****************************************************************/
>   
>   static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus)
> @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = {
>   	&dev_attr_panel_fhd.attr,
>   	&dev_attr_cores_enabled.attr,
>   	&dev_attr_cores_max.attr,
> +	&dev_attr_apu_mem.attr,
>   	&dev_attr_mini_led_mode.attr,
>   	&dev_attr_available_mini_led_mode.attr,
>   	NULL
> @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>   	else if (attr == &dev_attr_cores_enabled.attr
>   		|| attr == &dev_attr_cores_max.attr)
>   		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET);
> +	else if (attr == &dev_attr_apu_mem.attr)
> +		ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM);
>   	else if (attr == &dev_attr_mini_led_mode.attr)
>   		ok = asus->mini_led_dev_id != 0;
>   	else if (attr == &dev_attr_available_mini_led_mode.attr)
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 5a56e7e97785..efe608861e55 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -121,6 +121,9 @@
>    /* Maximum Intel E-core and P-core availability */
>   #define ASUS_WMI_DEVID_CORES_MAX	0x001200D3
>   
> +/* Set the memory available to the APU */
> +#define ASUS_WMI_DEVID_APU_MEM		0x000600C1
> +
>   /* MCU powersave mode */
>   #define ASUS_WMI_DEVID_MCU_POWERSAVE   0x001200E2
>