Re: [PATCH v4 18/18] platform/chrome: cros_ec_typec: Handle lack of HPD information

[Tzung-Bi Shih <tzungbi@kernel.org>]

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