On Mon, Feb 10, 2020 at 4:14 AM Kishon Vijay Abraham I <kishon@xxxxxx> wrote: > > Hi, > > On 10/02/20 5:00 PM, Alexandre Torgue wrote: > > Hi Kishon, > > > > On 2/10/20 9:08 AM, Kishon Vijay Abraham I wrote: > >> Hi Alexandre, > >> > >> On 07/02/20 12:27 PM, youling 257 wrote: > >>> test this diff, dwc3 work for my device, thanks. > >>> > >>> 2020-02-07 13:16 GMT+08:00, Kishon Vijay Abraham I <kishon@xxxxxx>: > >>>> Hi, > >>>> > >>>> On 06/02/20 7:09 PM, youling257 wrote: > >>>>> This patch cause "dwc3 dwc3.3.auto: failed to create device link to > >>>>> dwc3.3.auto.ulpi" problem. > >>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206435 > >>>> > >>>> I'm suspecting there is some sort of reverse dependency with dwc3 ULPI. > >>>> Can you try the following diff? > >>>> > >>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > >>>> index 2eb28cc2d2dc..397311dcb116 100644 > >>>> --- a/drivers/phy/phy-core.c > >>>> +++ b/drivers/phy/phy-core.c > >>>> @@ -687,7 +687,7 @@ struct phy *phy_get(struct device *dev, const char > >>>> *string) > >>>> > >>>> get_device(&phy->dev); > >>>> > >>>> - link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS); > >>>> + link = device_link_add(dev, &phy->dev, > >>>> DL_FLAG_SYNC_STATE_ONLY); > >>>> if (!link) { > >>>> dev_err(dev, "failed to create device link to %s\n", > >>>> dev_name(phy->dev.parent)); > >>>> @@ -802,7 +802,7 @@ struct phy *devm_of_phy_get(struct device *dev, > >>>> struct device_node *np, > >>>> return phy; > >>>> } > >>>> > >>>> - link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS); > >>>> + link = device_link_add(dev, &phy->dev, > >>>> DL_FLAG_SYNC_STATE_ONLY); Definitely don't use SYNC_STATE_ONLY. To give some context, drivers themselves are only expected to use STATELESS links. Only the driver core is supposed to use MANAGED (if you don't use STATELESS, it's MANAGED by default) links. And SYNC_STATE_ONLY makes sense only for MANAGED links. Also, as the SYNC_STATE_ONLY documentation says, it only affect sync_state() behavior and doesn't affect suspend/resume in any way. > >>>> if (!link) { > >>>> dev_err(dev, "failed to create device link to %s\n", > >>>> dev_name(phy->dev.parent)); > >>>> @@ -851,7 +851,7 @@ struct phy *devm_of_phy_get_by_index(struct device > >>>> *dev, struct device_node *np, > >>>> *ptr = phy; > >>>> devres_add(dev, ptr); > >>>> > >>>> - link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS); > >>>> + link = device_link_add(dev, &phy->dev, > >>>> DL_FLAG_SYNC_STATE_ONLY); > >>>> if (!link) { > >>>> dev_err(dev, "failed to create device link to %s\n", > >>>> dev_name(phy->dev.parent));Parent > >> > >> Can you check if this doesn't affect the suspend/resume ordering? > > > > With this fix, suspend/resume ordering is broken on my side. What do you > > think to keep the STATELESS flag and to only display a warn if > > "device_link_add" returns an error ? It's not "smart" but it could > > solved our issue. > > yeah, that sounds reasonable for now. Can you find out the dependencies > between dwc3 and ulpi and what exactly breaks. That would help Saravana > to suggest a fix? Yup, I don't have enough context of the dependencies here to suggest a good fix. But if device_link_add() is failing with STATELESS and not failing with SYNC_STATE_ONLY, I'm pretty sure you have a cyclic dependency between these devices when you create this link. Keep in mind that it can be a cycle involving more than 2 devices -- A -> B -> C -> A. And cycles don't make sense when you are trying to order suspend/resume. Looks like the new link is wrong or an existing link is wrong. If the dependencies are actually correct in hardware, then maybe your hardware device needs to be split into multiple devices so that you don't have cycles and can suspend in a meaningful way? Hope that helps. Thanks, Saravana
