Re: [PATCH] platform/x86: amd-pmc: Add support for AMD Spill to DRAM STB feature

["Limonciello, Mario" <mario.limonciello@amd.com>]

>> I don't think you followed my thought process here.
>>
>> If `amd_pmc_stb_debugfs_open_v2` is called then dev->msg_port is set to 1.
>>
>> So that means that any future call to `amd_pmc_send_cmd` (such as is
>> done for OS_HINT) will use the STD registers.  So doesn't that mean
>> OS_HINT is sent to STD registers?
>>
>> If STD registers support all the existing commands as the PMC registers
>> this is fine, otherwise I think there needs to be more logic to check
>> whether the command is PMC or STD related.
> 
> ah..I get your point now..

OK glad it makes sense and is a valid worry.

> 
>>
>> But you only allow size to be S2D_TELEMETRY_BYTES_MAX, shouldn't you be
>> flexible to a range of what the firmware returns rather than the single
>> value?
> 
> the value is derived after querying the FW, atleast by design today for
> yellow carp devices the FW team has confirmed that it shall remain 1M
> (even the next ASIC version too).

Very well - I suppose if this is fixed in the FW for YC when another 
ASIC changes this the driver can just change then too.

>>
>> I think those only take account when unloading the driver or unbinding
>> the device though, don't they?  So if devm_ioremap worked but kzalloc
>> failed then you call `amd_pmc_stb_debugfs_open_v2` again without unbind
>> or unload driver devm_ioremap will be called again too.
> 
> Makes sense. Will move them to a new routine maybe something like
> amd_pmc_s2d_init() and call that from the probe() so that should address
> this problem.

Yeap, sounds good.

> 
> shall respin a new revision.
> 
> Thanks,
> Shyam
> 
>>
>>>
>>> Thanks,
>>> Shyam
>>>
>>>>
>>>>> +
>>>>> +    memcpy_fromio(buf, dev->stb_virt_addr, S2D_TELEMETRY_BYTES_MAX);
>>>>> +
>>>>> +    filp->private_data = buf;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static ssize_t amd_pmc_stb_debugfs_read_v2(struct file *filp, char
>>>>> __user *buf, size_t size,
>>>>> +                       loff_t *pos)
>>>>> +{
>>>>> +    if (!filp->private_data)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    return simple_read_from_buffer(buf, size, pos, filp->private_data,
>>>>> +                    S2D_TELEMETRY_BYTES_MAX);
>>>>> +}
>>>>> +
>>>>> +static int amd_pmc_stb_debugfs_release_v2(struct inode *inode, struct
>>>>> file *filp)
>>>>> +{
>>>>> +    kfree(filp->private_data);
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct file_operations amd_pmc_stb_debugfs_fops_v2 = {
>>>>> +    .owner = THIS_MODULE,
>>>>> +    .open = amd_pmc_stb_debugfs_open_v2,
>>>>> +    .read = amd_pmc_stb_debugfs_read_v2,
>>>>> +    .release = amd_pmc_stb_debugfs_release_v2,
>>>>> +};
>>>>> +
>>>>>     static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct
>>>>> device *dev,
>>>>>                      struct seq_file *s)
>>>>>     {
>>>>> @@ -350,9 +428,14 @@ static void amd_pmc_dbgfs_register(struct
>>>>> amd_pmc_dev *dev)
>>>>>         debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir,
>>>>> dev,
>>>>>                     &amd_pmc_idlemask_fops);
>>>>>         /* Enable STB only when the module_param is set */
>>>>> -    if (enable_stb)
>>>>> -        debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>>>>> -                    &amd_pmc_stb_debugfs_fops);
>>>>> +    if (enable_stb) {
>>>>> +        if (dev->cpu_id == AMD_CPU_ID_YC)
>>>>> +            debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>>>>> +                        &amd_pmc_stb_debugfs_fops_v2);
>>>>> +        else
>>>>> +            debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
>>>>> +                        &amd_pmc_stb_debugfs_fops);
>>>>> +    }
>>>>>     }
>>>>>     #else
>>>>>     static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>>>>> @@ -392,26 +475,47 @@ static int amd_pmc_setup_smu_logging(struct
>>>>> amd_pmc_dev *dev)
>>>>>       static void amd_pmc_dump_registers(struct amd_pmc_dev *dev)
>>>>>     {
>>>>> -    u32 value;
>>>>> +    u32 value, message, argument, response;
>>>>> +
>>>>> +    if (dev->msg_port) {
>>>>> +        message = AMD_S2D_REGISTER_MESSAGE;
>>>>> +        argument = AMD_S2D_REGISTER_ARGUMENT;
>>>>> +        response = AMD_S2D_REGISTER_RESPONSE;
>>>>> +    } else {
>>>>> +        message = AMD_PMC_REGISTER_MESSAGE;
>>>>> +        argument = AMD_PMC_REGISTER_ARGUMENT;
>>>>> +        response = AMD_PMC_REGISTER_RESPONSE;
>>>>> +    }
>>>>>     -    value = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_RESPONSE);
>>>>> +    value = amd_pmc_reg_read(dev, response);
>>>>>         dev_dbg(dev->dev, "AMD_PMC_REGISTER_RESPONSE:%x\n", value);
>>>>>     -    value = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_ARGUMENT);
>>>>> +    value = amd_pmc_reg_read(dev, argument);
>>>>>         dev_dbg(dev->dev, "AMD_PMC_REGISTER_ARGUMENT:%x\n", value);
>>>>>     -    value = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_MESSAGE);
>>>>> +    value = amd_pmc_reg_read(dev, message);
>>>>>         dev_dbg(dev->dev, "AMD_PMC_REGISTER_MESSAGE:%x\n", value);
>>>>>     }
>>>>>       static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32
>>>>> *data, u8 msg, bool ret)
>>>>>     {
>>>>>         int rc;
>>>>> -    u32 val;
>>>>> +    u32 val, message, argument, response;
>>>>>           mutex_lock(&dev->lock);
>>>>> +
>>>>> +    if (dev->msg_port) {
>>>>> +        message = AMD_S2D_REGISTER_MESSAGE;
>>>>> +        argument = AMD_S2D_REGISTER_ARGUMENT;
>>>>> +        response = AMD_S2D_REGISTER_RESPONSE;
>>>>> +    } else {
>>>>> +        message = AMD_PMC_REGISTER_MESSAGE;
>>>>> +        argument = AMD_PMC_REGISTER_ARGUMENT;
>>>>> +        response = AMD_PMC_REGISTER_RESPONSE;
>>>>> +    }
>>>>> +
>>>>>         /* Wait until we get a valid response */
>>>>> -    rc = readx_poll_timeout(ioread32, dev->regbase +
>>>>> AMD_PMC_REGISTER_RESPONSE,
>>>>> +    rc = readx_poll_timeout(ioread32, dev->regbase + response,
>>>>>                     val, val != 0, PMC_MSG_DELAY_MIN_US,
>>>>>                     PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>>>>>         if (rc) {
>>>>> @@ -420,16 +524,16 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev
>>>>> *dev, u32 arg, u32 *data, u8 msg,
>>>>>         }
>>>>>           /* Write zero to response register */
>>>>> -    amd_pmc_reg_write(dev, AMD_PMC_REGISTER_RESPONSE, 0);
>>>>> +    amd_pmc_reg_write(dev, response, 0);
>>>>>           /* Write argument into response register */
>>>>> -    amd_pmc_reg_write(dev, AMD_PMC_REGISTER_ARGUMENT, arg);
>>>>> +    amd_pmc_reg_write(dev, argument, arg);
>>>>>           /* Write message ID to message ID register */
>>>>> -    amd_pmc_reg_write(dev, AMD_PMC_REGISTER_MESSAGE, msg);
>>>>> +    amd_pmc_reg_write(dev, message, msg);
>>>>>           /* Wait until we get a valid response */
>>>>> -    rc = readx_poll_timeout(ioread32, dev->regbase +
>>>>> AMD_PMC_REGISTER_RESPONSE,
>>>>> +    rc = readx_poll_timeout(ioread32, dev->regbase + response,
>>>>>                     val, val != 0, PMC_MSG_DELAY_MIN_US,
>>>>>                     PMC_MSG_DELAY_MIN_US * RESPONSE_REGISTER_LOOP_MAX);
>>>>>         if (rc) {
>>>>> @@ -442,7 +546,7 @@ static int amd_pmc_send_cmd(struct amd_pmc_dev
>>>>> *dev, u32 arg, u32 *data, u8 msg,
>>>>>             if (ret) {
>>>>>                 /* PMFW may take longer time to return back the data */
>>>>>                 usleep_range(DELAY_MIN_US, 10 * DELAY_MAX_US);
>>>>> -            *data = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_ARGUMENT);
>>>>> +            *data = amd_pmc_reg_read(dev, argument);
>>>>>             }
>>>>>             break;
>>>>>         case AMD_PMC_RESULT_CMD_REJECT_BUSY:
>>>>
>>