Re: [PATCH v2] platform/x86: amd-pmc: Add AMD platform support for S2Idle

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Oct 29, 2020 at 10:57 AM Shyam Sundar S K
<Shyam-sundar.S-k@amd.com> wrote:
>
> AMD Power Management Controller driver aka. amd-pmc driver is the

aka. -> a.k.a. or AKA

> controller which is meant for final S2Idle transaction that goes to the

for the final

> PMFW running on the AMD SMU (System Management Unit) responsible for
> tuning of the VDD.
>
> Once all the monitored list or the idle constraints are met, this driver
> would go and set the OS_HINT (meaning all the devices have reached to
> their lowest state possible) via the SMU mailboxes.
>
> This driver would also provide some debug capabilities via debugfs.

...

> +F:     drivers/platform/x86/amd-pmc.c
> +F:     drivers/platform/x86/amd-pmc.h

Can be one entry.

...

> +       help
> +         The driver provides support for AMD Power Management Controller
> +         primarily responsible for S2Idle transactions that are driven from
> +         a platform firmware running on SMU. This driver also provides debug

a debug

> +         mechanism to investigate the S2Idle transactions and failures.

How will the module be called?

...

> +static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
> +{
> +       struct dentry *dir;

What is the point of having the temporary variable?

> +       dir = debugfs_create_dir("amd_pmc", NULL);
> +       dev->dbgfs_dir = dir;
> +       debugfs_create_file("smu_fw_info", 0644, dir, dev, &smu_fw_info_fops);
> +}

...

> +static int amd_pmc_poll_smu_response(struct amd_pmc_dev *dev)
> +{
> +       int rc = 0, index;

Redundant assignment.

> +       for (index = 0; index < RESPONSE_REGISTER_LOOP_MAX; index++) {
> +               rc = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_RESPONSE);

> +               if (rc != 0)
> +                       break;

if (rc)
  return rc;

> +
> +               usleep_range(PMC_MSG_DELAY_MIN, PMC_MSG_DELAY_MAX);
> +       }
> +
> +       return rc;
> +}

Seems like home grown readx_poll_timeout().

...

> +       int rc = 0;

Redundant assignment.

...

> +       /* Wait until the response register is non-zero */

This comment is confusing.

I guess you rather need to return an error code from the called function...

> +       rc = amd_pmc_poll_smu_response(dev);
> +       if (!rc) {
> +               dev_err(dev->dev, "failed to talk to SMU\n");

> +               return -EIO;

...and propagate it here.

> +       }

...

> +       if (arg) {
> +               /* Wait until the response register is non-zero */
> +               rc = amd_pmc_poll_smu_response(dev);

> +               if (!rc) {

As above. This is an unusual pattern in the kernel.

> +                       dev_err(dev->dev, "failed to talk to SMU\n");
> +                       return -EIO;
> +               }

> +               /* If message register is OK, then SMU has finished processing
> +                * the message, and then read back from AMD_PMC_REGISTER_ARGUMENT
> +                */

Comment style!

> +               rc = amd_pmc_reg_read(dev, AMD_PMC_REGISTER_ARGUMENT);

And where is rc going?

> +       }
> +
> +       return 0;

...

> +#ifdef CONFIG_PM_SLEEP

__maybe_unused is slightly better.

...

> +static int amd_pmc_suspend(struct device *dev)
> +{
> +       struct amd_pmc_dev *pdev = dev_get_drvdata(dev);

> +       int rc = 0;

What is the point?

> +       rc = amd_pmc_send_cmd(pdev, 1, 0);
> +       if (rc)
> +               dev_err(pdev->dev, "suspend failed\n");
> +
> +       amd_pmc_dump_registers(pdev);
> +
> +       return 0;
> +}
> +
> +static int amd_pmc_resume(struct device *dev)
> +{
> +       struct amd_pmc_dev *pdev = dev_get_drvdata(dev);

> +       int rc = 0;

Ditto.

> +       rc = amd_pmc_send_cmd(pdev, 0, 0);
> +       if (rc)
> +               dev_err(pdev->dev, "resume failed\n");
> +
> +       amd_pmc_dump_registers(pdev);
> +
> +       return 0;
> +}
> +#endif

...

> +       rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> +       if (!rdev || (!(rdev->vendor == PCI_VENDOR_ID_AMD) &&
> +                     (rdev->device == AMD_CPU_ID_PCO ||
> +                       rdev->device == AMD_CPU_ID_RN))) {
> +               return -ENODEV;
> +       }

Can you rather create an additional PCI ID table and simple use pci_match_id()?

> +       err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO);
> +       if (err) {
> +               dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
> +               return err;
> +       }

Returning positive error codes?!

...

> +       dev->smu_base = ioremap(base_addr, AMD_PMC_MAPPING_SIZE);

devm_ioremap()?

> +       if (!dev->smu_base)
> +               return -ENOMEM;
> +
> +       dev->regbase = ioremap(base_addr + AMD_PMC_BASE_ADDR_OFFSET, AMD_PMC_MAPPING_SIZE);

Ditto.

> +       if (!dev->regbase) {
> +               iounmap(dev->smu_base);
> +               return -ENOMEM;
> +       }

...

> +               .acpi_match_table = ACPI_PTR(amd_pmc_acpi_ids),

It depends on ACPI. What is the point of ACPI_PTR?

...

> +

Redundant blank line

> +module_platform_driver(amd_pmc_driver);

...

> +++ b/drivers/platform/x86/amd-pmc.h

Who is going to use this file?

...

> +#define AMD_PMC_BASE_ADDR_HI_MASK      0xFFF00000L
> +#define AMD_PMC_BASE_ADDR_LO_MASK      0x0000FFFFL

You included bits.h and not using it, why?

...

> +#define PMC_MSG_DELAY_MIN              100
> +#define PMC_MSG_DELAY_MAX              200

Units?

...

> +enum amd_pmc_def {
> +       msg_test = 0x01,
> +       msg_os_hint_pco,
> +       msg_os_hint_rn,
> +};

Hmm... Perhaps capital letters?

-- 
With Best Regards,
Andy Shevchenko