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

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

> > 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: