diff mbox series

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

Message ID 20230518161150.92959-1-mario.limonciello@amd.com
State Accepted
Commit a7fbfd44c0204f0629288edfd0d77829edb4a2f8
Headers show
Series [v2] usb: typec: ucsi: Mark dGPUs as DEVICE scope | expand

Commit Message

Mario Limonciello May 18, 2023, 4:11 p.m. UTC
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>
---
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(-)

Comments

Heikki Krogerus May 19, 2023, 8:12 a.m. UTC | #1
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,
Sebastian Reichel May 20, 2023, 5:24 p.m. UTC | #2
Hi,

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

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> 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
>
Gregor Riepl May 21, 2023, 9:23 a.m. UTC | #3
> 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`.

Is this a given fact?

I could imagine that a dGPU device would be capable of feeding power 
back into the system, for example when an external docking station or 
display device has a power supply connected to it.

Although I don't think it's possible (or safe) with a desktop system, 
maybe a portable computer with a discrete GPU would be able to do this?
Andi Shyti June 14, 2023, 12:16 a.m. UTC | #4
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>

Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Thanks,
Andi
Wolfram Sang June 14, 2023, 8:47 a.m. UTC | #5
On Wed, Jun 14, 2023 at 02:16:21AM +0200, Andi Shyti wrote:
> 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>
> 
> Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Applied to for-next, thanks!
diff mbox series

Patch

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;
 	}