On Thu 07 Oct 03:17 PDT 2021, Heikki Krogerus wrote: > Hi guys, > > On Wed, Oct 06, 2021 at 01:26:35PM -0700, Prashant Malani wrote: > > (CC+ Heikki) > > > > Hi, > > > > On Wed, Oct 6, 2021 at 8:19 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > On Tue, Oct 5, 2021 at 7:27 PM Bjorn Andersson > > > <bjorn.andersson@xxxxxxxxxx> wrote: > > > > > > > > > > For reference, this is how I thought one is supposed to tie the Type-C > > > > > > controller to the display driver: > > > > > > https://lore.kernel.org/all/20211005022451.2037405-1-bjorn.andersson@xxxxxxxxxx/ > > > > > > > > > > OK, so I looked at that a bit. Fair warning that I've never looked at > > > > > the type C code before today so anything I say could be totally wrong! > > > > > :-) > > > > > > > > > > ...but I _think_ you're abusing the "mux" API for this. I think a type > > > > > C port can have exactly 1 mux, right? Right now you are claiming to be > > > > > _the_ mux in the DP driver, but what about for other alt modes? If > > > > > those wanted to be notified about similar things it would be > > > > > impossible because you're already _the_ mux, right? > > > > > > > > > > > > > I actually don't think so, because I acquire the typec_mux handle by the > > > > means of: > > > > > > > > mux_desc.svid = USB_TYPEC_DP_SID; > > > > mux_desc.mode = USB_TYPEC_DP_MODE; > > > > alt_port->mux = fwnode_typec_mux_get(fwnode, &mux_desc); > > > > > > Hrm, I guess I need to go find that code. Ah, I see it in your WIP > > > tree, but not posted anywhere. :-P The only code I can see calling > > > fwnode_typec_mux_get() is `drivers/platform/chrome/cros_ec_typec.c`. > > > In that code it passes NULL for the mux_desc and I'm nearly certain > > > that it just handles one "mux" per connector despite the fact that it > > > handles lots of different types of alternate modes. That doesn't mean > > > that the cros_ec implementation is correct / finalized, but it's a > > > reference point. > > > > > > > > > > And in the DisplayPort node I provide svid = /bits/ 16 <0xff01>; > > > > > > > > So I will be able to reference multiple different altmode > > > > implementors using this scheme. > > > > > > OK, so I'm trying to grok this more. Let's see. > > > > > > I'm looking at ucsi_glink_probe() and looking at the matching dts in > > > your WIP tree [1] in "sc8180x-lenovo-flex-5g.dts" OK, so: > > > > > > 1. It's looping once per _connector_ by looping with > > > `device_for_each_child_node(dev, fwnode)`. > > > > > > 2. For each connector, it has exactly one `alt_port` structure. > > > > > > 3. For each `alt_port` structure it has exactly one `mux`. > > > > > > ...so currently with your WIP tree there is one "mux" per type C connector. > > > > > > > > > Perhaps what you're saying, though, is that the UCSI code in your WIP > > > tree can/should be changed to support more than one mux per port. Then > > > I guess it would have logic figuring out what muxes to notify about > > > which things? ...and I guess that would mean that it's currently a bug > > > that the ucsi_altmode_enable_usb() notifies "the DP type C mux" about > > > USB changes? > > > > > > > > > > > I _think_ a mux is supposed to be something more like > > > > > `drivers/phy/rockchip/phy-rockchip-typec.c` (though that code predates > > > > > the type C framework we're looking at here). There the phy can do all > > > > > the work of remuxing things / flipping orientation / etc. I don't > > > > > think it's a requirement that every SoC be able to do this remuxing > > > > > itself but (if memory serves) rk3399 implemented it so we didn't have > > > > > to do it on the TCPC and could use a cheaper solution there. > > > > > > > > > > > > > I'm afraid I don't see how this interacts with a display controller. > > > > > > This was actually kinda my point. ;-) Specifically I think > > > `phy-rockchip-typec.c` is the thing that's supposed to be a "mux". I > > > think your display controller isn't a mux. Yeah, it's handy that muxes > > > get told about DP HPD status, but that doesn't mean it's the right > > > abstraction for you to implement. In my mental model, it's the same as > > > implementing your "i2c" controller with a "pinctrl" driver. :-P > > > > > > > > > > It > > > > seems more like it's the phy side of things, what we have split between > > > > the Type-C controller and the QMP phy to set the pins in the right > > > > state. > > > > > > > > > In any case, my point is that I think there is supposed to be a > > > > > _single_ mux per port that handles reassigning pins and that's what > > > > > this API is for. > > > > > > > > > > > > > If that's the case things such as typec_mux_match() is just completely > > > > backwards. > > > > > > Yeah, I have no explanation for typec_mux_match(). Let me see if I can > > > lure some type C folks into this discussion. > > > > This aligns with the model I have in my mind (not that that is > > necessarily the right one). > > I took that matching code to be meant to handle cases where the > > firmware doesn't explicitly > > define a "mode-switch" for the port (and so we look at the SVIDs > > listed in the Mux fwnode descriptor). > > > > The matcher code does suggest there could be a mux for each alternate > > mode. But then, how does the > > bus code know which mux to set [2] ? In that code, the struct altmode > > has a pointer to the struct typec_mux, but I > > don't see where that pointer is assigned. I assumed that it was set to > > whatever the mux node of the > > Type C port was whenever the port driver registered its altmodes for > > each port, but I can't substantiate > > that assumption in code. > > > > Heikki, do you have any guidance regarding what the expected usage is > > here? One typec_mux struct per type C port? Or > > 1 typec_mux per altmode per port? > > I didn't go over the whole thread, so I may have misunderstood > something, but I don't think this has anything to do with muxes. The > mux should not be a problem for the DRM side under no circumstance. > Like Doug said, the mux API is being abused here. > No need to read up on the thread, your answer further confirms the understanding gained in a lengthy offline chat we had yesterday afternoon as well. > HPD was one use case here, so I'll try to explain how that happens... > > If the USB Type-C connector is in DP alt mode, then ideally your USB > Type-C controller/port driver has registered the partner device DP alt > mode the moment it detected that the partner supports that mode, and > that partner DP alt mode will have then been bind to the DP alt mode > driver: > > drivers/usb/typec/altmodes/displayport.c > > After that, if the DP alt mode driver sees HPD - HPD is message > signalled in DP alt mode (in case some of you guys didn't know) - the > DP alt mode driver notifies the DRM connector about it by calling > this function: > > void drm_connector_oob_hotplug_event(struct fwnode_handle *connector_fwnode); > > If your USB Type-C controller/port driver does not yet register the DP > alt mode, the it's responsible of handling HPD separately by calling > drm_connector_oob_hotplug_event() on its own. > The drm_connector_oob_hotplug_event() didn't exist when I tried to get this working earlier this year and I couldn't figure out what the intended design was to feed the HPD information into our DP driver. Misplacing the typec_mux made all the pieces fall in place and it looked good, but I now agree that the typec_mux should be used to mux in/out the DP PHY on the pads as a result of the PD negotiation and then separate of that the HPD signals should be sent towards the DRM driver using drm_connector_oob_hotplug_event() - hopefully by reusing the displayport altmode driver, but I still need to figure out how to incorporate that in my custom TypeC controller driver. > Either way, the only thing needed here is description of the > connection between the USB Type-C connector and the DisplayPort in > firmware - the mux is not relevant here. There are no DT bindings > defined for that AFAIK (or are there?), but presumable you want to use > OF graph with DT. Right now the DP alt mode driver does not try to > find the connection from device graph (so OF graph), but it should not > be a problem to add support for it. > I'll poke around and see what's missing to get drm_connector_oob_hotplug_event() work in my model. > The one thing that I still don't understand though is, if the typec_mux is used by the typec controller to inform _the_ mux about the function to be used, what's up with the complexity in typec_mux_match()? This is what lead me to believe that typec_mux was enabling/disabling individual altmodes, rather just flipping the physical switch at the bottom. Thanks, Bjorn > > > > > ...so I will still assert that the right thing to do is to have a > > > > > drm_bridge for the type c connector and _that's_ what should be > > > > > sending HPD. > > > > > > > > > > > > > That still implies that all the current typec_mux code got it all wrong > > > > and should be thrown out. If you instead consider that you have a Type-C > > > > controller that upon switching DisplayPort on/off calls typec_mux_set() > > > > to inform the functions that things has changed then all the current > > > > code makes sense. > > > > > > > > It also maps nicely to how the TypeC controller would call > > > > typec_switch_set() to inform, in our case the QMP phy that the > > > > orientation has switched. > > > > > > > > > > > > It seems reasonable to have some common helper code that registers the > > > > typec_mux and turn its notifications into HPD notifications to the > > > > display code, but I still think that should live in the DRM framework, > > > > separate from the USB code. > > > > > > I think I'm going to step back and hope that the experts can chime in. > > > > > > > > > [1] https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819 > > [2]: https://elixir.bootlin.com/linux/v5.15-rc4/source/drivers/usb/typec/bus.c#L27 > > > > > > > > -Doug > > thanks, > > -- > heikki
