Re: [PATCH v2 1/1] i2c: designware: Consolidate PM ops

["Goswami, Sanket" <Sanket.Goswami@xxxxxxx>]

Adding Shyam (as he asked me to check this change on AMD systems)

On 8/27/2024 8:30 PM, Andy Shevchenko wrote:
> We have the same (*) PM ops in the PCI and plaform drivers.
> Instead, consolidate that PM ops under exported variable and
> deduplicate them.
>
> *)
> With the subtle ACPI and P-Unit behaviour differences in PCI case.
> But this is not a problem as for ACPI we need to take care of the
> P-Unit semaphore anyway and calling PM ops for PCI makes sense as
> it might provide specific operation regions in ACPI (however there
> are no known devices on market that are using it with PCI enabled I2C).
> Note, the clocks are not in use in the PCI case.
>
> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Tested-by: Sanket Goswami <Sanket.Goswami@xxxxxxx>

Thanks,
Sanket

> ---
>
> This is just a split of a single patch from bigger series [1].
> It was made in no functionality change manner, except the PCI case
> described above. But since it touches PM and it was an area where
> regressions were observed I would like to have a formal Tested-by
> from AMD people to make sure we don't break their setup /
> configurations.
>                                                                                                                   
> Link: https://lore.kernel.org/linux-i2c/20231207141653.2785124-1-andriy.shevchenko@xxxxxxxxxxxxxxx/ [1]
>
> v2: fixed rebase issue (LKP)
>
>   drivers/i2c/busses/i2c-designware-common.c  | 62 ++++++++++++++++++++
>   drivers/i2c/busses/i2c-designware-core.h    |  3 +
>   drivers/i2c/busses/i2c-designware-pcidrv.c  | 44 +-------------
>   drivers/i2c/busses/i2c-designware-platdrv.c | 64 +--------------------
>   4 files changed, 68 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index b60c55587e48..fb65fe6d8122 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -21,6 +21,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/pm.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/property.h>
>   #include <linux/regmap.h>
> @@ -736,5 +737,66 @@ void i2c_dw_disable(struct dw_i2c_dev *dev)
>   }
>   EXPORT_SYMBOL_GPL(i2c_dw_disable);
>   
> +static int i2c_dw_prepare(struct device *device)
> +{
> +	/*
> +	 * If the ACPI companion device object is present for this device,
> +	 * it may be accessed during suspend and resume of other devices via
> +	 * I2C operation regions, so tell the PM core and middle layers to
> +	 * avoid skipping system suspend/resume callbacks for it in that case.
> +	 */
> +	return !has_acpi_companion(device);
> +}
> +
> +static int i2c_dw_runtime_suspend(struct device *device)
> +{
> +	struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> +	if (dev->shared_with_punit)
> +		return 0;
> +
> +	i2c_dw_disable(dev);
> +	i2c_dw_prepare_clk(dev, false);
> +
> +	return 0;
> +}
> +
> +static int i2c_dw_suspend(struct device *device)
> +{
> +	struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> +	i2c_mark_adapter_suspended(&dev->adapter);
> +
> +	return i2c_dw_runtime_suspend(device);
> +}
> +
> +static int i2c_dw_runtime_resume(struct device *device)
> +{
> +	struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> +	if (!dev->shared_with_punit)
> +		i2c_dw_prepare_clk(dev, true);
> +
> +	dev->init(dev);
> +
> +	return 0;
> +}
> +
> +static int i2c_dw_resume(struct device *device)
> +{
> +	struct dw_i2c_dev *dev = dev_get_drvdata(device);
> +
> +	i2c_dw_runtime_resume(device);
> +	i2c_mark_adapter_resumed(&dev->adapter);
> +
> +	return 0;
> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(i2c_dw_dev_pm_ops) = {
> +	.prepare = pm_sleep_ptr(i2c_dw_prepare),
> +	LATE_SYSTEM_SLEEP_PM_OPS(i2c_dw_suspend, i2c_dw_resume)
> +	RUNTIME_PM_OPS(i2c_dw_runtime_suspend, i2c_dw_runtime_resume, NULL)
> +};
> +
>   MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter core");
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 723d599cca93..c6bd6f65a2d3 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -15,6 +15,7 @@
>   #include <linux/dev_printk.h>
>   #include <linux/errno.h>
>   #include <linux/i2c.h>
> +#include <linux/pm.h>
>   #include <linux/regmap.h>
>   #include <linux/types.h>
>   
> @@ -341,6 +342,8 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev);
>   int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev);
>   u32 i2c_dw_func(struct i2c_adapter *adap);
>   
> +extern const struct dev_pm_ops i2c_dw_dev_pm_ops;
> +
>   static inline void __i2c_dw_enable(struct dw_i2c_dev *dev)
>   {
>   	dev->status |= STATUS_ACTIVE;
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index dd188ccd961e..04377533f3ae 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -19,6 +19,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
> +#include <linux/pm.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/power_supply.h>
>   #include <linux/sched.h>
> @@ -194,47 +195,6 @@ static struct dw_pci_controller dw_pci_controllers[] = {
>   	},
>   };
>   
> -static int __maybe_unused i2c_dw_pci_runtime_suspend(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	i2c_dw_disable(i_dev);
> -	return 0;
> -}
> -
> -static int __maybe_unused i2c_dw_pci_suspend(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	i2c_mark_adapter_suspended(&i_dev->adapter);
> -
> -	return i2c_dw_pci_runtime_suspend(dev);
> -}
> -
> -static int __maybe_unused i2c_dw_pci_runtime_resume(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	return i_dev->init(i_dev);
> -}
> -
> -static int __maybe_unused i2c_dw_pci_resume(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -	int ret;
> -
> -	ret = i2c_dw_pci_runtime_resume(dev);
> -
> -	i2c_mark_adapter_resumed(&i_dev->adapter);
> -
> -	return ret;
> -}
> -
> -static const struct dev_pm_ops i2c_dw_pm_ops = {
> -	SET_SYSTEM_SLEEP_PM_OPS(i2c_dw_pci_suspend, i2c_dw_pci_resume)
> -	SET_RUNTIME_PM_OPS(i2c_dw_pci_runtime_suspend, i2c_dw_pci_runtime_resume, NULL)
> -};
> -
>   static const struct property_entry dgpu_properties[] = {
>   	/* USB-C doesn't power the system */
>   	PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
> @@ -402,7 +362,7 @@ static struct pci_driver dw_i2c_driver = {
>   	.probe		= i2c_dw_pci_probe,
>   	.remove		= i2c_dw_pci_remove,
>   	.driver         = {
> -		.pm     = &i2c_dw_pm_ops,
> +		.pm	= pm_ptr(&i2c_dw_dev_pm_ops),
>   	},
>   	.id_table	= i2c_designware_pci_ids,
>   };
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index e49c68c6e142..285ba4c1803f 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -29,7 +29,6 @@
>   #include <linux/reset.h>
>   #include <linux/sched.h>
>   #include <linux/slab.h>
> -#include <linux/suspend.h>
>   #include <linux/units.h>
>   
>   #include "i2c-designware-core.h"
> @@ -339,67 +338,6 @@ static void dw_i2c_plat_remove(struct platform_device *pdev)
>   	reset_control_assert(dev->rst);
>   }
>   
> -static int dw_i2c_plat_prepare(struct device *dev)
> -{
> -	/*
> -	 * If the ACPI companion device object is present for this device, it
> -	 * may be accessed during suspend and resume of other devices via I2C
> -	 * operation regions, so tell the PM core and middle layers to avoid
> -	 * skipping system suspend/resume callbacks for it in that case.
> -	 */
> -	return !has_acpi_companion(dev);
> -}
> -
> -static int dw_i2c_plat_runtime_suspend(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	if (i_dev->shared_with_punit)
> -		return 0;
> -
> -	i2c_dw_disable(i_dev);
> -	i2c_dw_prepare_clk(i_dev, false);
> -
> -	return 0;
> -}
> -
> -static int dw_i2c_plat_suspend(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	i2c_mark_adapter_suspended(&i_dev->adapter);
> -
> -	return dw_i2c_plat_runtime_suspend(dev);
> -}
> -
> -static int dw_i2c_plat_runtime_resume(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	if (!i_dev->shared_with_punit)
> -		i2c_dw_prepare_clk(i_dev, true);
> -
> -	i_dev->init(i_dev);
> -
> -	return 0;
> -}
> -
> -static int dw_i2c_plat_resume(struct device *dev)
> -{
> -	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> -
> -	dw_i2c_plat_runtime_resume(dev);
> -	i2c_mark_adapter_resumed(&i_dev->adapter);
> -
> -	return 0;
> -}
> -
> -static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
> -	.prepare = pm_sleep_ptr(dw_i2c_plat_prepare),
> -	LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
> -	RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, dw_i2c_plat_runtime_resume, NULL)
> -};
> -
>   static const struct of_device_id dw_i2c_of_match[] = {
>   	{ .compatible = "snps,designware-i2c", },
>   	{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
> @@ -442,7 +380,7 @@ static struct platform_driver dw_i2c_driver = {
>   		.name	= "i2c_designware",
>   		.of_match_table = dw_i2c_of_match,
>   		.acpi_match_table = dw_i2c_acpi_match,
> -		.pm	= pm_ptr(&dw_i2c_dev_pm_ops),
> +		.pm	= pm_ptr(&i2c_dw_dev_pm_ops),
>   	},
>   	.id_table = dw_i2c_platform_ids,
>   };