On Wed, Sep 04, 2024 at 02:45:36PM -0700, Stephen Boyd wrote:
> Quoting Tzung-Bi Shih (2024-09-04 02:36:45)
> > On Sat, Aug 31, 2024 at 09:06:56PM -0700, Stephen Boyd wrote:
> > > +static void cros_typec_inject_hpd(struct cros_typec_data *typec,
> > > + struct ec_response_usb_pd_mux_info *resp,
> > > + struct cros_typec_port *port)
> > > +{
> > [...]
> > > + /*
> > > + * Only read the mux GPIO setting if we need to change the active port.
> > > + * Otherwise, an active port is already set and HPD going high or low
> > > + * doesn't change the muxed port until DP mode is exited.
> > > + */
> > > + if (!typec->active_dp_port) {
> >
> > Given that cros_typec_inject_hpd() is called before `typec->active_dp_port`
> > would be set (from previous patch "platform/chrome: ... Support DP muxing"),
> > would it possibly wrongly fall into here at the beginning? (E.g.:
> > cros_typec_probe() -> cros_typec_port_update() -> cros_typec_configure_mux()
> > -> cros_typec_inject_hpd().)
>
> We wouldn't get here if 'hpd_asserted' is false though. We want to fall
Ack, I overlooked that.
> > > [...]
> > > + /* Inject HPD from the GPIO state if EC firmware is broken. */
> > > + if (typec->hpd_asserted)
> > > + resp->flags |= USB_PD_MUX_HPD_LVL;
> >
> > `typec->hpd_asserted` is shared between all typec->ports[...]. Would it be
> > possible that a HPD is asserted for another port but not current `port`?
> > E.g.: cros_typec_inject_hpd() for port 2 and cros_typec_dp_bridge_hpd_notify()
> > gets called due to port 1 at the same time?
>
> I'd like to avoid synchronizing the hpd notify and this injection code,
> if that's what you're asking. Thinking about this though, I've realized
> that it's broken even when HPD is working on the EC. Consider this
> scenario with two type-c ports C0 and C1:
>
> [...]
I understood it more: originally, I was wondering if it needs an array
`typec->hpd_asserted[...]` for storing HPD for each port. But, no.
What cros_typec_dp_bridge_hpd_notify() get is just a connected/disconnected
signal. It kicks off cros_typec_port_work() for finding which port is
supposed to trigger the event (with some logic with `active_dp_port`,
`mux_gpio`, and `hpd_asserted`).
Curious about one more scenario, is it possible:
Initially, no DP port and no mux is using:
active_dp_port = NULL
hpd_asserted = false
mux_gpio = NULL
CPU A CPU B
------------------------------------------------------------------------------
cros_typec_port_work()
cros_typec_port_update(port_num=0)
[C0 connected]
cros_typec_dp_bridge_hpd_notify()
hpd_asserted = true
cros_typec_port_update(port_num=1)
cros_typec_configure_mux(port_num=1)
cros_typec_inject_hpd()
active_dp_port = C1
