Re: [PATCH v2] usb: typec: ucsi: Mark dGPUs as DEVICE scope

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Mario,

On Thu, May 18, 2023 at 11:11:50AM -0500, Mario Limonciello wrote:
> power_supply_is_system_supplied() checks whether any power
> supplies are present that aren't batteries to decide whether
> the system is running on DC or AC.  Downstream drivers use
> this to make performance decisions.
> 
> Navi dGPUs include an UCSI function that has been exported
> since commit 17631e8ca2d3 ("i2c: designware: Add driver
> support for AMD NAVI GPU").
> 
> This UCSI function registers a power supply since commit
> 992a60ed0d5e ("usb: typec: ucsi: register with power_supply class")
> but this is not a system power supply.
> 
> As the power supply for a dGPU is only for powering devices connected
> to dGPU, create a device property to indicate that the UCSI endpoint
> is only for the scope of `POWER_SUPPLY_SCOPE_DEVICE`.
> 
> Link: https://lore.kernel.org/lkml/20230516182541.5836-2-mario.limonciello@amd.com/
> Reviewed-by: Evan Quan <evan.quan@amd.com>
> Tested-by: Evan Quan <evan.quan@amd.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Look good to me, but if there is anything else that has to be changed,
please consider modifying the ucsi driver part in its own patch.
Otherwise, FWIW:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> v1->v2:
>  * Drop patch 1, merged into a maintainers tree
>  * Fix title
>  * Add tags
>  * Fix terminators
> ---
>  drivers/i2c/busses/i2c-designware-pcidrv.c | 13 ++++++++++++-
>  drivers/i2c/busses/i2c-nvidia-gpu.c        |  3 +++
>  drivers/usb/typec/ucsi/psy.c               | 14 ++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 782fe1ef3ca1..61d7a27aa070 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -20,6 +20,7 @@
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/power_supply.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  
> @@ -234,6 +235,16 @@ static const struct dev_pm_ops i2c_dw_pm_ops = {
>  	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),
> +	{}
> +};
> +
> +static const struct software_node dgpu_node = {
> +	.properties = dgpu_properties,
> +};
> +
>  static int i2c_dw_pci_probe(struct pci_dev *pdev,
>  			    const struct pci_device_id *id)
>  {
> @@ -325,7 +336,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>  	}
>  
>  	if ((dev->flags & MODEL_MASK) == MODEL_AMD_NAVI_GPU) {
> -		dev->slave = i2c_new_ccgx_ucsi(&dev->adapter, dev->irq, NULL);
> +		dev->slave = i2c_new_ccgx_ucsi(&dev->adapter, dev->irq, &dgpu_node);
>  		if (IS_ERR(dev->slave))
>  			return dev_err_probe(dev->dev, PTR_ERR(dev->slave),
>  					     "register UCSI failed\n");
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> index a8b99e7f6262..26622d24bb1b 100644
> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -14,6 +14,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/power_supply.h>
>  
>  #include <asm/unaligned.h>
>  
> @@ -261,6 +262,8 @@ MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
>  static const struct property_entry ccgx_props[] = {
>  	/* Use FW built for NVIDIA GPU only */
>  	PROPERTY_ENTRY_STRING("firmware-name", "nvidia,gpu"),
> +	/* USB-C doesn't power the system */
> +	PROPERTY_ENTRY_U8("scope", POWER_SUPPLY_SCOPE_DEVICE),
>  	{ }
>  };
>  
> diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> index 56bf56517f75..384b42267f1f 100644
> --- a/drivers/usb/typec/ucsi/psy.c
> +++ b/drivers/usb/typec/ucsi/psy.c
> @@ -27,8 +27,20 @@ static enum power_supply_property ucsi_psy_props[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_CURRENT_MAX,
>  	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_SCOPE,
>  };
>  
> +static int ucsi_psy_get_scope(struct ucsi_connector *con,
> +			      union power_supply_propval *val)
> +{
> +	u8 scope = POWER_SUPPLY_SCOPE_UNKNOWN;
> +	struct device *dev = con->ucsi->dev;
> +
> +	device_property_read_u8(dev, "scope", &scope);
> +	val->intval = scope;
> +	return 0;
> +}
> +
>  static int ucsi_psy_get_online(struct ucsi_connector *con,
>  			       union power_supply_propval *val)
>  {
> @@ -194,6 +206,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
>  		return ucsi_psy_get_current_max(con, val);
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  		return ucsi_psy_get_current_now(con, val);
> +	case POWER_SUPPLY_PROP_SCOPE:
> +		return ucsi_psy_get_scope(con, val);
>  	default:
>  		return -EINVAL;
>  	}
> -- 
> 2.34.1

thanks,

-- 
heikki