On 2/2/2022 7:54 PM, Limonciello, Mario wrote:
> On 2/2/2022 00:03, Shyam Sundar S K wrote:
>>
>>
>> On 2/2/2022 10:14 AM, Limonciello, Mario wrote:
>>> On 1/27/2022 04:09, Sanket Goswami wrote:
>>>> Spill to DRAM functionality is a feature that allows STB (Smart Trace
>>>> Buffer) to spill data from SRAM into DRAM on some future AMD ASICs. The
>>>> size allocated for STB is more than the earlier SoC's which helps to
>>>> collect more tracing and telemetry data.
>>>>
>>>> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>>>> Signed-off-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>
>>>> ---
>>>> drivers/platform/x86/amd-pmc.c | 132
>>>> +++++++++++++++++++++++++++++----
>>>> 1 file changed, 118 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/amd-pmc.c
>>>> b/drivers/platform/x86/amd-pmc.c
>>>> index 4c72ba68b315..3040f50627c6 100644
>>>> --- a/drivers/platform/x86/amd-pmc.c
>>>> +++ b/drivers/platform/x86/amd-pmc.c
>>>> @@ -41,6 +41,16 @@
>>>> #define AMD_PMC_STB_PMI_0 0x03E30600
>>>> #define AMD_PMC_STB_PREDEF 0xC6000001
>>>> +/* STB S2D(Spill to DRAM) has different message port offset */
>>>> +#define STB_SPILL_TO_DRAM 0xBE
>>>> +#define AMD_S2D_REGISTER_MESSAGE 0xA20
>>>> +#define AMD_S2D_REGISTER_RESPONSE 0xA80
>>>> +#define AMD_S2D_REGISTER_ARGUMENT 0xA88
>>>> +
>>>> +/* STB Spill to DRAM Parameters */
>>>> +#define S2D_TELEMETRY_BYTES_MAX 0x100000
>>>> +#define S2D_TELEMETRY_DRAMBYTES_MAX 0x1000000
>>>
>>> Why have two separate definitions for the same value? Is there a case
>>> that these will ever be different?
>>
>> Values are different right? _BYTES_MAX is 1M and _DRAM_MAX is 16M
>
> Sorry, my mistake when I looked - yes.
>
>>
>>>
>>>> +
>>>> /* Base address of SMU for mapping physical address to virtual
>>>> address */
>>>> #define AMD_PMC_SMU_INDEX_ADDRESS 0xB8
>>>> #define AMD_PMC_SMU_INDEX_DATA 0xBC
>>>> @@ -95,6 +105,13 @@ enum amd_pmc_def {
>>>> MSG_OS_HINT_RN,
>>>> };
>>>> +enum s2d_arg {
>>>> + S2D_TELEMETRY_SIZE = 0x01,
>>>> + S2D_PHYS_ADDR_LOW,
>>>> + S2D_PHYS_ADDR_HIGH,
>>>> + S2D_NUM_SAMPLES,
>>>> +};
>>>> +
>>>> struct amd_pmc_bit_map {
>>>> const char *name;
>>>> u32 bit_mask;
>>>> @@ -119,7 +136,9 @@ static const struct amd_pmc_bit_map soc15_ip_blk[]
>>>> = {
>>>> struct amd_pmc_dev {
>>>> void __iomem *regbase;
>>>> void __iomem *smu_virt_addr;
>>>> + void __iomem *stb_virt_addr;
>>>> void __iomem *fch_virt_addr;
>>>> + bool msg_port;
>>>> u32 base_addr;
>>>> u32 cpu_id;
>>>> u32 active_ips;
>>>> @@ -236,6 +255,65 @@ static const struct file_operations
>>>> amd_pmc_stb_debugfs_fops = {
>>>> .release = amd_pmc_stb_debugfs_release,
>>>> };
>>>> +static int amd_pmc_stb_debugfs_open_v2(struct inode *inode, struct
>>>> file *filp)
>>>> +{
>>>> + struct amd_pmc_dev *dev = filp->f_inode->i_private;
>>>> + u32 *buf, phys_addr_low, phys_addr_hi, size, samples;
>>>> + u64 stb_phys_addr;
>>>> +
>>>> + /* Spill to DRAM feature uses separate SMU message port */
>>>> + dev->msg_port = 1;
>>>
>>> This message port is only used for STD right? Existing messages for
>>> version, OS_HINT and the like should go through the old port.
>>
>> msg_port is a flag to decide whether to use the regular PMC port or the
>> new STB port defined for Spill-to-DRAM feature.
>>
>> Both are complementary. Spill-to-DRAM will only be enabled in Yellow
>> Carp and future ASICs and lower than that the new message port will not
>> be enabled.
>>
>> The decision of whether to use v2 or v1 versions of debugfs is being
>> taken in the debugfs creation with a cpuid check and hence I believe the
>> versions/OS_HINT will not collide.
>>
>> Also, this change has to tested several iterations on older and newer
>> platforms before it was posted here. So, based that this we should
>> ignore any such concerns around the OS_HINT.
>> >>
>>> You don't seem to ever unset this, so I think if the STB was opened
>>> before S0i3 was invoked I would think it would cause the OS_HINT message
>>> to be sent to the wrong port wouldn't it?
>>
>> I tried to cover this in the above details. No, it would not cause HINT
>> to be sent to wrong message port.
>
> 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..
>
>>
>>>
>>> It might be safer to modify amd_pmc_send_cmd to have an extra argument
>>> of which port to use in each invocation. Then only in debugfs_v2
>>> operations you can use this port.
>>>
>>>> +
>>>> + amd_pmc_send_cmd(dev, S2D_TELEMETRY_SIZE, &size,
>>>> STB_SPILL_TO_DRAM, 1);
>>>> + if (size != S2D_TELEMETRY_BYTES_MAX)
>>>> + return -EIO;
>>>
>>> What if the firmware was configured to use less than 16MB? Shouldn't
>>> you be able to respect that?
>>
>> That is why there is a seperate call to fetch the S2D(Spill-to-DRAM)
>> size which the FW has configured it for notifying it to the driver.
>
> 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).
>
>>
>> Btw, though 16M is the max DRAM carved out space for 16M, but currently
>> only 1M is allocated to get the full STB dumps from DRAM space and this
>> by design. We have the same change on the windows side too.
>>
>>>
>>> I would think you instead want to have a boundary check on the "max" to
>>> support.
>>
>> Above explanation should hopefully address this.
>>
>>>
>>>> +
>>>> + /* Get STB DRAM address */
>>>> + amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_LOW, &phys_addr_low,
>>>> STB_SPILL_TO_DRAM, 1);
>>>> + amd_pmc_send_cmd(dev, S2D_PHYS_ADDR_HIGH, &phys_addr_hi,
>>>> STB_SPILL_TO_DRAM, 1);
>>>> +
>>>> + /* Get the number of samples present in the STB buffer */
>>>> + amd_pmc_send_cmd(dev, S2D_NUM_SAMPLES, &samples,
>>>> STB_SPILL_TO_DRAM, 1);
>>>> + stb_phys_addr = ((u64)phys_addr_hi << 32 | phys_addr_low);
>>>> +
>>>> + dev->stb_virt_addr = devm_ioremap(dev->dev, stb_phys_addr,
>>>> S2D_TELEMETRY_DRAMBYTES_MAX);
>>>> + if (!dev->stb_virt_addr)
>>>> + return -ENOMEM;
>>>> +
>>>> + buf = kzalloc(S2D_TELEMETRY_BYTES_MAX, GFP_KERNEL);
>>>> + if (!buf)
>>>> + return -ENOMEM;
>>>
>>> Shouldn't you clear the iomapping for dev->stb_virt_addr on this
>>> failure?
>>
>> I believe, that is why we use managed devm_*() APIs which is supposed to
>> take care of reducing the reference counts ?
>
> 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.
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:
>>>
>
