| Message ID | 20221124102056.393220-3-treapking@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Register Type-C mode-switch in DP bridge endpoints | expand |
On Thu, Nov 24, 2022 at 06:20:51PM +0800, Pin-yen Lin wrote: > From: Prashant Malani <pmalani@chromium.org> > > When using OF graph, the fw_devlink code will create links between the > individual port driver (cros-ec-typec here) and the parent device for > a Type-C switch (like mode-switch). Since the mode-switch will in turn > have the usb-c-connector (i.e the child of the port driver) as a > supplier, fw_devlink will not be able to resolve the cyclic dependency > correctly. > > As a result, the mode-switch driver probe() never runs, so mode-switches > are never registered. Because of that, the port driver probe constantly > fails with -EPROBE_DEFER, because the Type-C connector class requires all > switch devices to be registered prior to port registration. > > To break this deadlock and allow the mode-switch registration to occur, > purge all the usb-c-connector nodes' absent suppliers. This eliminates > the connector as a supplier for a switch and allows it to be probed. ... > + /* > + * OF graph may have set up some device links with switches, since connectors have their > + * own compatible. Purge these to avoid a deadlock in switch probe (the switch mistakenly > + * assumes the connector is a supplier). > + */ A bit too long lines... > + if (dev->of_node) Why do you need this check? > + device_for_each_child_node(dev, fwnode) > + fw_devlink_purge_absent_suppliers(fwnode);
Hi Andy, On Thu, Nov 24, 2022 at 8:25 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Nov 24, 2022 at 06:20:51PM +0800, Pin-yen Lin wrote: > > From: Prashant Malani <pmalani@chromium.org> > > > > When using OF graph, the fw_devlink code will create links between the > > individual port driver (cros-ec-typec here) and the parent device for > > a Type-C switch (like mode-switch). Since the mode-switch will in turn > > have the usb-c-connector (i.e the child of the port driver) as a > > supplier, fw_devlink will not be able to resolve the cyclic dependency > > correctly. > > > > As a result, the mode-switch driver probe() never runs, so mode-switches > > are never registered. Because of that, the port driver probe constantly > > fails with -EPROBE_DEFER, because the Type-C connector class requires all > > switch devices to be registered prior to port registration. > > > > To break this deadlock and allow the mode-switch registration to occur, > > purge all the usb-c-connector nodes' absent suppliers. This eliminates > > the connector as a supplier for a switch and allows it to be probed. > > ... > > > + /* > > + * OF graph may have set up some device links with switches, since connectors have their > > + * own compatible. Purge these to avoid a deadlock in switch probe (the switch mistakenly > > + * assumes the connector is a supplier). > > + */ > > A bit too long lines... I'll fix this in v7. > > > + if (dev->of_node) > > Why do you need this check? We use this check to make sure only platforms using OF have their links purged. I'm not sure if this should also be done on x86 platforms. Best regards, Pin-yen > > > + device_for_each_child_node(dev, fwnode) > > + fw_devlink_purge_absent_suppliers(fwnode); > > -- > With Best Regards, > Andy Shevchenko > >
Hi Andy, Thanks for taking a look at this patch. Pin-Yen beat me to the punch with comment responses, but I'll add mine anyway. On Thu, Nov 24, 2022 at 4:25 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > ... > > > + /* > > + * OF graph may have set up some device links with switches, since connectors have their > > + * own compatible. Purge these to avoid a deadlock in switch probe (the switch mistakenly > > + * assumes the connector is a supplier). > > + */ > > A bit too long lines... They are within the 100 character limit [1] which is followed elsewhere in the driver; has something changed recently to make that invalid? > > > + if (dev->of_node) > > Why do you need this check? This issue only arises when using DT for this device. So the rationale is we don't need to perform this step on systems that don't use DT. Best regards, -Prashant [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c index 2a7ff14dc37e..f74e01d18ef3 100644 --- a/drivers/platform/chrome/cros_ec_typec.c +++ b/drivers/platform/chrome/cros_ec_typec.c @@ -382,6 +382,15 @@ static int cros_typec_init_ports(struct cros_typec_data *typec) return -EINVAL; } + /* + * OF graph may have set up some device links with switches, since connectors have their + * own compatible. Purge these to avoid a deadlock in switch probe (the switch mistakenly + * assumes the connector is a supplier). + */ + if (dev->of_node) + device_for_each_child_node(dev, fwnode) + fw_devlink_purge_absent_suppliers(fwnode); + /* DT uses "reg" to specify port number. */ port_prop = dev->of_node ? "reg" : "port-number"; device_for_each_child_node(dev, fwnode) {