Re: [PATCH v1 3/3] usb: dwc3-generic: Add Starfive JHB100 USB 2.0 device controller and adapter

[Minda Chen]

> 
> (sorry for the delay)
> 
> On Fri, Sep 18, 2026, Minda Chen wrote:
> > Add PHY or PHY adapter iomem field in struct dwc3_gerneric for
> > dwc3 generic platform to add PHY/PHY adapter setting.
> >
> > Also Add Starfive JHB100 USB 2.0 device controller generic init
> > function to remap PHY adapter io memory region and add adapter
> > pre-run/stop function.
> >
> > JHB100 BMC soc contains Synopsys dwc3 multi-port host controller.
> > And export its xhci controller to host server through PCIe. xhci
> > contains 8 ports and each connect with one dwc3 device controller
> > through an PHY adapter. Server host USB subsystem includes one
> > multi-port dwc3 USB host, multiple dwc3 device controllers and their
> > adapters.
> >
> > While USB adapter 2.0 enable bit is set, the connect status bit of
> > xhci port stat register will be asserted. If the usb enumeration is
> > not ready for USB gadget, The xhci will fail with enumerate usb device.
> > So The JHB100 USB device controller adapter require to set/clear USB
> > 2.0 port enable while usb gadget start/stop usb device enumeration.
> > This can be treated as a hardware quirk.
> >
> > Signed-off-by: Minda Chen <minda.chen@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/usb/dwc3/dwc3-generic-plat.c | 47
> > +++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-generic-plat.c
> > b/drivers/usb/dwc3/dwc3-generic-plat.c
> > index ca69ac0eb07c..e3394205a519 100644
> > --- a/drivers/usb/dwc3/dwc3-generic-plat.c
> > +++ b/drivers/usb/dwc3/dwc3-generic-plat.c
> > @@ -22,12 +22,15 @@
> >  #define EIC7700_HSP_AXI_LP_XM_CSYSREQ	BIT(0)
> >  #define EIC7700_HSP_AXI_LP_XS_CSYSREQ	BIT(16)
> >
> > +#define JHB100_ADAPTER_PORT_EN		BIT(20)
> > +
> >  struct dwc3_generic {
> >  	struct device		*dev;
> >  	struct dwc3		dwc;
> >  	struct clk_bulk_data	*clks;
> >  	int			num_clocks;
> >  	struct reset_control	*resets;
> > +	void __iomem		*phy_regs; /* PHY or PHY adapter regs */
> 
> The name should be generic. Can we just use "regs" and drop the comment?
> 
> >  };
> >
> >  struct dwc3_generic_config {
> > @@ -42,6 +45,43 @@ static void dwc3_generic_reset_control_assert(void
> *data)
> >  	reset_control_assert(data);
> >  }
> >
> > +static void dwc3_jhb100_pre_run_stop(struct dwc3 *dwc3, bool start)
> 
> Can we keep the dwc parameter name consistent with the rest of the dwc3 code.
> 
> > +{
> > +	struct dwc3_generic *data = to_dwc3_generic(dwc3);
> > +	u32	val;
> > +
> > +	if (start) {
> > +		val = readl(data->phy_regs);
> > +		val |= JHB100_ADAPTER_PORT_EN;
> > +		writel(val, data->phy_regs);
> > +	} else {
> > +		val = readl(data->phy_regs);
> > +		val &= ~JHB100_ADAPTER_PORT_EN;
> > +		writel(val, data->phy_regs);
> > +	}
> 
> Minor nit, simplify to this:
> 	val = readl(data->phy_regs);
> 	if (start)
> 		val |= JHB100_ADAPTER_PORT_EN;
> 	else
> 		val &= ~JHB100_ADAPTER_PORT_EN;
> 
> 	writel(val, data->phy_regs);
> 
> > +}
> > +
> > +struct dwc3_glue_ops jhb100_glue_ops = {
> 
> Should this be static?
> 
> static struct dwc3_glue_ops jhb100_glue_ops
> 
> > +	.pre_run_stop = dwc3_jhb100_pre_run_stop, };
> > +
> > +static int dwc3_jhb100_init(struct dwc3_generic *dwc3g) {
> > +	struct platform_device *pdev = to_platform_device(dwc3g->dev);
> > +
> > +	dwc3g->phy_regs = devm_platform_ioremap_resource_byname(pdev,
> "adapter");
> > +	if (IS_ERR(dwc3g->phy_regs)) {
> 
> What if this fails for reasons other than resource not found? Please properly
> check for error code.
> 
> > +		dwc3g->phy_regs = NULL;
> > +		/* Generic bmc USB with PHY*/
> 
> Fix the comment missing space.
> 
> > +		return 0;
> > +	}
> > +
> > +	/* usb device controller with adapter */
> > +	dwc3g->dwc.glue_ops = &jhb100_glue_ops;
> > +
> > +	return 0;
> > +}
> > +
> >  static int dwc3_eic7700_init(struct dwc3_generic *dwc3g)  {
> >  	struct device *dev = dwc3g->dev;
> > @@ -231,12 +271,17 @@ static const struct dwc3_generic_config
> eic7700_dwc3 =  {
> >  	.properties = DWC3_DEFAULT_PROPERTIES,  };
> >
> > +static const struct dwc3_generic_config jhb100_dwc3 =  {
> > +	.init = dwc3_jhb100_init,
> > +	.properties = DWC3_DEFAULT_PROPERTIES, };
> > +
> >  static const struct of_device_id dwc3_generic_of_match[] = {
> >  	{ .compatible = "spacemit,k1-dwc3", &spacemit_k1_dwc3},
> >  	{ .compatible = "spacemit,k3-dwc3", },
> >  	{ .compatible = "fsl,ls1028a-dwc3", &fsl_ls1028_dwc3},
> >  	{ .compatible = "eswin,eic7700-dwc3", &eic7700_dwc3},
> > -	{ .compatible = "starfive,jhb100-dwc3", },
> > +	{ .compatible = "starfive,jhb100-dwc3", &jhb100_dwc3},
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, dwc3_generic_of_match);
> > --
> > 2.17.1
> >
> 
> The rest looks good to me.
> 
> Thanks,
> Thinh

Thanks for review! I will follow the comments to modify and sent v2!