Re: [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 08 Jul 03:17 PDT 2021, Bryan O'Donoghue wrote:

> On 08/07/2021 04:54, Bjorn Andersson wrote:
> > Bryan had a previous patch where the glue layer was notified about role
> > switching (iirc) and as soon as we hit a probe deferal in the core
> > driver we'd dereference some pointer in the glue layer. I don't find the
> > patch right now, but I suspect it might have been caused by the same
> > platform_get_drvdata() as we see in qcom_dwc3_resume_irq().
> 
> Here
> 
> https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/
> 
> and here
> 
> https://lore.kernel.org/linux-usb/20200311191501.8165-8-bryan.odonoghue@linaro.org/
> 

Now that I dug through the code I remembered why this didn't work.

You do:
	dwc = platform_get_drvdata(qcom->dwc3);

In order to be able to register the callback in the notifier chain that
you added to struct dwc3, but while qcom->dwc3 is a valid struct
platform_device, it might not have probed yet and "dwc" becomes NULL,
which you then dereferenced in dwc3_role_switch_notifier_register().

So we need a mechanism that passes that callback/notifier that has a
life cycle matching that of the glue device.

> one thing about that I don't think is right now in retrospect is having to
> find a DT connector in the core, meaning it incorrectly assumes you have a
> node named "connector" as a child of dwc3-core
> 
> https://lore.kernel.org/linux-usb/158463604559.152100.9219030962819234620@swboyd.mtv.corp.google.com/
> 
> Having done some work with TCPM on pm8150b silicon I see what Stephen was
> saying about that
> 
> That's one solid reason I like the USB role-switch API - because it gets you
> out of the business of trying to discern from dwc3-qcom if dwc3-core has
> role-switching turned on by iterating through its range of DT nodes and
> looking for a special one named "connector"
> 
> The initial and imperfect solution I had for that looked like this
> 
> if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {}
> 
> Wesley had another iteration on that that was a little better
> 
> https://lore.kernel.org/linux-usb/20201009082843.28503-4-wcheng@codeaurora.org/
> 
> +static int dwc3_qcom_connector_check(struct fwnode_handle *fwnode)
> +{
> +	if (fwnode && (!fwnode_property_match_string(fwnode, "compatible",
> +						     "gpio-usb-b-connector") ||
> +	    !fwnode_property_match_string(fwnode, "compatible",
> +					  "usb-c-connector")))
> +		return 1;
> +
> +	return 0;
> +}
> 
> All we are really doing there is replicating functionality that the
> role-switch API already provides
> 

But isn't this role switching interaction (both B and C) already part of
the core/drd, so if we can just get the drd to invoke
dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the
extcon code from the qcom glue as well)?

Regards,
Bjorn