On Wed, Nov 4, 2020 at 5:31 PM Shyam Sundar S K
<Shyam-sundar.S-k@xxxxxxx> wrote:
>
> AMD Power Management Controller driver a.k.a. amd-pmc driver is the
> controller which is meant for the final S2Idle transaction that goes to
> the 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.
...
> +AMD PMC DRIVER
> +M: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> +L: platform-driver-x86@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: drivers/platform/x86/amd-pmc*
Better to leave dot: amd-pmc.*
...
> +/* SMU communcation registers */
communication.
...
> +#define AMD_PMC_BASE_ADDR_HI_MASK 0xFFF00000L
> +#define AMD_PMC_BASE_ADDR_LO_MASK 0x0000FFFFL
This is the same, however I have commented on it.
Use proper macro from bits.h.
...
> +/* SMU Response Codes */
> +#define AMD_PMC_RESULT_OK 0x1
0x01
> +#define AMD_PMC_RESULT_FAILED 0xFF
> +#define AMD_PMC_RESULT_CMD_UNKNOWN 0xFE
> +#define AMD_PMC_RESULT_CMD_REJECT_PREREQ 0xFD
> +#define AMD_PMC_RESULT_CMD_REJECT_BUSY 0xFC
Keep them sorted.
...
> +#define PMC_MSG_DELAY_MIN_USECS 100
_US is enough.
...
> +static struct amd_pmc_dev {
> + u32 base_addr;
> + u32 cpu_id;
> + void __iomem *regbase;
If you make this a first member the below IO accessor will be
optimized by the compiler better.
> + void __iomem *smu_base;
This probably will be second for the sake of consistency.
> + struct device *dev;
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> + struct dentry *dbgfs_dir;
> +#endif /* CONFIG_DEBUG_FS */
> +} pmc;
Can you split structure declaration and a variable of the new type?
...
> + /* Wait until we get a valid response */
> + rc = readx_poll_timeout(ioread32, dev->regbase + AMD_PMC_REGISTER_RESPONSE,
> + val, val > 0, PMC_MSG_DELAY_MIN_USECS,
> + PMC_MSG_DELAY_MIN_USECS * RESPONSE_REGISTER_LOOP_MAX);
> + if (rc) {
> + dev_err(dev->dev, "failed to talk to SMU\n");
> + return -ETIMEDOUT;
Why not
return rc;
?
> + }
...
> + dev->cpu_id = rdev->device;
> + 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;
What are you doing?
The PCI API can return all possible error codes: Linux native
(negative values), 0 as success, positive as special PCI error codes.
Look at the pci.h and find a way how to handle this properly.
> + }
...
> + dev->smu_base = ioremap(base_addr, AMD_PMC_MAPPING_SIZE);
devm_ioremap() ?
Why ignored this comment?
> + 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;
> + }
--
With Best Regards,
Andy Shevchenko
