On Mon, Nov 13, 2023, at 12:15 PM, Mark Pearson wrote:
> Thanks Ilpo,
>
> On Mon, Nov 13, 2023, at 11:59 AM, Ilpo Järvinen wrote:
>> On Mon, 13 Nov 2023, Mark Pearson wrote:
>>
>>> Some new Thinkpads have a new improved performance mode available.
>>> Add support to make this mode usable.
>>>
>>> To avoid having to create a new profile, just use the improved performance
>>> mode in place of the existing performance mode, when available.
>>>
>>> Tested on P14s AMD G4 AMD.
>>>
>>> Signed-off-by: Mark Pearson <mpearson-lenovo@xxxxxxxxx>
>>> ---
>>> Changes in v2: updated implementation for DYTC_UP_SUPPORT define
>>> Changes in v3:
>>> - Add in missing BIT for define, somehow lost in previous commit
>>> - Cosmetic clean-ups
>>>
>>> drivers/platform/x86/thinkpad_acpi.c | 28 ++++++++++++++++++++++++++++
>>> 1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
>>> index ad460417f901..3a9d2cc71b6a 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -10136,6 +10136,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>>
>>> #define DYTC_CMD_SET 1 /* To enable/disable IC function mode */
>>> #define DYTC_CMD_MMC_GET 8 /* To get current MMC function and mode */
>>> +#define DYTC_CMD_UP_CAP 0xA /* To get Ultra-performance capability */
>>> #define DYTC_CMD_RESET 0x1ff /* To reset back to default */
>>>
>>> #define DYTC_CMD_FUNC_CAP 3 /* To get DYTC capabilities */
>>> @@ -10152,6 +10153,7 @@ static struct ibm_struct proxsensor_driver_data = {
>>>
>>> #define DYTC_FUNCTION_STD 0 /* Function = 0, standard mode */
>>> #define DYTC_FUNCTION_CQL 1 /* Function = 1, lap mode */
>>> +#define DYTC_FUNCTION_TMS 9 /* Function = 9, TMS mode */
>>> #define DYTC_FUNCTION_MMC 11 /* Function = 11, MMC mode */
>>> #define DYTC_FUNCTION_PSC 13 /* Function = 13, PSC mode */
>>> #define DYTC_FUNCTION_AMT 15 /* Function = 15, AMT mode */
>>> @@ -10163,11 +10165,14 @@ static struct ibm_struct proxsensor_driver_data = {
>>> #define DYTC_MODE_MMC_LOWPOWER 3 /* Low power mode */
>>> #define DYTC_MODE_MMC_BALANCE 0xF /* Default mode aka balanced */
>>> #define DYTC_MODE_MMC_DEFAULT 0 /* Default mode from MMC_GET, aka balanced */
>>> +#define DYTC_NOMODE 0xF /* When Function does not have a mode */
>>>
>>> #define DYTC_MODE_PSC_LOWPOWER 3 /* Low power mode */
>>> #define DYTC_MODE_PSC_BALANCE 5 /* Default mode aka balanced */
>>> #define DYTC_MODE_PSC_PERFORM 7 /* High power mode aka performance */
>>>
>>> +#define DYTC_UP_SUPPORT BIT(8) /* Ultra-performance (TMS) mode support */
>>> +
>>> #define DYTC_ERR_MASK 0xF /* Bits 0-3 in cmd result are the error result */
>>> #define DYTC_ERR_SUCCESS 1 /* CMD completed successful */
>>>
>>> @@ -10185,6 +10190,7 @@ static enum platform_profile_option dytc_current_profile;
>>> static atomic_t dytc_ignore_event = ATOMIC_INIT(0);
>>> static DEFINE_MUTEX(dytc_mutex);
>>> static int dytc_capabilities;
>>> +static bool dytc_ultraperf_cap;
>>> static bool dytc_mmc_get_available;
>>> static int profile_force;
>>>
>>> @@ -10355,6 +10361,17 @@ static int dytc_profile_set(struct platform_profile_handler *pprof,
>>> if (err)
>>> goto unlock;
>>>
>>> + /* Set TMS mode appropriately (enable for performance), if available */
>>> + if (dytc_ultraperf_cap) {
>>> + int cmd;
>>> +
>>> + cmd = DYTC_SET_COMMAND(DYTC_FUNCTION_TMS, DYTC_NOMODE,
>>> + profile == PLATFORM_PROFILE_PERFORMANCE);
>>> + err = dytc_command(cmd, &output);
>>> + if (err)
>>> + return err;
>>> + }
>>> +
>>> if (dytc_capabilities & BIT(DYTC_FC_MMC)) {
>>> if (profile == PLATFORM_PROFILE_BALANCED) {
>>> /*
>>> @@ -10429,6 +10446,7 @@ static struct platform_profile_handler dytc_profile = {
>>> static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>> {
>>> int err, output;
>>> + int cmd;
>>>
>>> /* Setup supported modes */
>>> set_bit(PLATFORM_PROFILE_LOW_POWER, dytc_profile.choices);
>>> @@ -10484,6 +10502,16 @@ static int tpacpi_dytc_profile_init(struct ibm_init_struct *iibm)
>>> dbg_printk(TPACPI_DBG_INIT, "No DYTC support available\n");
>>> return -ENODEV;
>>> }
>>> + err = dytc_command(DYTC_CMD_UP_CAP, &output);
>>
>> Hmm, are you missing error handling here?
>>
> Doh....yes, it should check that. My bad.
>
> I'll hold off a day or two on the next patch so I'm not spamming the
> list in case there is other feedback, and do a fix for that.
> I don't want the driver to error out on this - but it shouldn't check
> (and potentially enable) the feature if that register read fails. I
> will go and double check on some older platforms too as a sanity check.
>
> Thanks for the review.
Just an update, as it's taking me longer to get what I thought would be the final version of this patch done.
I did some regression testing on other platforms, as a sanity check, and found on the Z16 G1 that it is reporting this feature as supported, but unfortunately it doesn't work (it switches to low power mode)
This is a FW issue so I'm asking the FW team for clarification on why I'm seeing this, why it isn't working, and how to fix it.
This is going to take some time unfortunately, so this patch will be delayed a bit. Apologies to anybody waiting for this improvement.
Thanks
Mark
