On Mon, Oct 01, 2018 at 04:23:31PM +0200, Hans de Goede wrote: > On 29-09-18 16:26, Yu Wang wrote: > > The USB PHY mux switch depend on both USB2/USB3 PHY state and xHCI/xDCI > > controller state. The role can't be switched if related states haven't > > satisfied. That is why we need to poll the DUAL_ROLE_CFG1 to check if > > the role switched successful or not. > > > > So the SW_IDPIN and SW_VBUS_VALID bits can't determine the current > > acting role. > > > > This patch changes the logic for getting role logic. > > I guess this matches up with the recent patches to change the > role-switching on non Cherry Trail devices to use the DRD_CONFIG bits > instead of the SW_IDPIN bit? > > AFAIK under normal circumstances with our current code, > DUAL_ROLE_CFG1 will always follow SW_IDPIN, so I'm not entirly sure > what you are trying to fix here? So if I've understood correctly, there are certain conditions inside both the xHCI and the DWC3 (which is here called xDCI) blocks that have to be met before the internal mux state actually gets changed. That means that there is no guarantee that the SW_IDPIN bit tells the actual state of the mux. > IOW what problem are you seeing without this patch and how does > this fix it ? If there is a change that we may in some situations read a wrong state for the mux, then that is a problem. But it would be good to explain the actual case that you are seeing in the commit message. And this patch should probable also be marked as a fix for the original commit that introduced the driver. > If this is related to the patch to use the DRD_CONFIG bits on > non Cherry Trail devices, then please first submit a new version > of the patch for that which leaves the functionality on CHT > devices as is (as discussed before). I'm not sure if this is directly related to that, but Yu, can you confirm that? I'm guessing that this is indirectly related to that patch. DRD_CONFIG bits apparently force the state of the mux, ignoring the state of xHCI and DWC3, so by using them the problem of reading wrong state for they mux could never happen. In any case, the rationale for the patch sounds perfectly reasonable to me. Even if we need to use the DRD_CONFIG bits in some cases, IMO we still should rely on the CFG1 to get the real state of the mux. Thanks, -- heikki
