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@xxxxxxx/
> Reviewed-by: Evan Quan <evan.quan@xxxxxxx>
> Tested-by: Evan Quan <evan.quan@xxxxxxx>
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
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@xxxxxxxxxxxxxxx>
> ---
> 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
