Re: [PATCH v4 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Wed, Mar 04, 2026, Ingo Rohloff wrote:
> Hello Thinh,
> 
> > > +static void dwc3_ulpi_detect_config(struct dwc3 *dwc)  
> > 
> > We can rename this to dwc3_ulpi_get_properties() and declare it in
> > core.h. Then we can have the dwc3_get_properties() call this function.
> 
> I tested this and this doesn't work:
> At the time dwc3_get_properties() runs, the DWC3 controller hasn't been
> initialized at all yet.
> 
> Any access to a DWC3 hardware register fails (I think the clocks to the
> DWC3 IP are not even enabled at that point of time) and the kernel crashes.

Ah, that's right.

> 
> To read out the ULPI PHY Vendor/Product ID at least the ULPI
> interface of the DWC3 controller must be up and running.
> 
> > This will ensure that the order of the setting of the GUSB2PHYCFG is
> > done in dwc3_hs_phy_setup(), which is prior to registering the ulpi phy.
> 
> I originally had the same idea: Make sure GUSB2PHYCFG settings are
> correct before I do any access to the ULPI PHY; that's why I originally
> used a device tree property.
> 
> But because I now use the Vendor/Product ID, this doesn't work;
> instead what I implemented right now does this:
> 
> - First setup the ULPI interface to be able to access the ULPI PHY
> - Check what ULPI PHY model is connected (via Vendor/Product ID)
> - Apply further workarounds depending on the detected ULPI PHY model.
> 
> The setting of the XCVRDLY in GUSB2PHYCFG only has an effect once you
> enable the USB connection.
> 

Right. I guess we can't avoid separating this step.

> 
> > > +static void dwc3_ulpi_apply_config(struct dwc3 *dwc)
> > > +{
> > >    ...
> >
> > This should be done in dwc3_hs_phy_setup(). See more comments about this
> > below.
> 
> The problem is: As far as I can tell, dwc3_hs_phy_setup() has to run first,
> to setup the DWC3 controller to access the ULPI PHY at all.
> 
> Right at the beginning the code in dwc3_hs_phy_setup() reads:
> 
> 	/* Select the HS PHY interface */
> 	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
> 	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
> 
> I think that's also the reason why dwc3_core_init() uses this order
> 
> 	ret = dwc3_phy_setup(dwc);
> 	...
> 	if (!dwc->ulpi_ready) {
> 		ret = dwc3_core_ulpi_init(dwc);
> 
> You first have to make sure that dwc3_phy_setup() has setup the interfaces
> to the PHYs (both High-Speed and Super-Speed) before trying to
> access the HS PHY.
> 
> 
> > > +static void dwc3_ulpi_detect_config(struct dwc3 *dwc) 
> > > +{
> > > +	u16 product_id;
> > > +	u16 vendor_id;
> > > +	int ret;
> > > +
> > > +	/* Test the interface */
> > > ...
> > 
> > Do we need to check for data integrity here? That check will be done
> > during ulpi init, where it has proper checks and can fail properly
> > there.
> 
> Thanks!
> OMG I am stupid :-):
> I misunderstood the code of ulpi_register_interface():
> I thought ulpi_register_interface() would only do this check and read out
> the Vendor/Product ID, if there is a matching device tree node, but it
> turns out it will always do the check and read out the
> Vendor/Product ID, even if there is no matching device tree node.
> 
> This in turn means: I don't even have to replicate any code, 
> because ulpi_register_interface() will already read out the
> ULPI PHYs Vendor/Product ID.
> 
> If drivers/usb/dwc3/core.c includes "linux/ulpi/driver.h" the code in
> core.c already has access to the ULPI PHY Vendor/Product ID via
>    dwc->ulpi->id.vendor
>    dwc->ulpi->id.product
> 
> Which means: I directly can use this to enable the XCVRDLY in GUSB2PHYCFG
> if the specific Vendor/Product ID is found.
> 
> What do you think?
> 

Sounds good! But can we have the checking for Vendor/Product ID in
ulpi.c instead of core.c? I want to keep the core.c generic.

Thanks,
Thinh