Re: [PATCH v1 2/2] usb: dwc3: Add Renesas R-Car Gen5 DWC3 xHCI USB controller glue

On 8/5/26 2:48 AM, Thinh Nguyen wrote:

Hello Thinh,

[...]

>> +static int dwc3_rcar_gen5_init(struct dwc3_rcar_gen5_priv *priv)
>> +{
>> +	/*
>> +	 * The datasheet describes initialization procedure without full
>> +	 * information about the registers. Therefore, the source code is
>> +	 * based on the bare metal code shared by the board team.
>> +	 */
>> +	writew(0x211, priv->base + 0x26);
>> +
>> +	/* USB3 does not need additional register programming. */
>> +	if (priv->use_usb3_flow)
>> +		return 0;
>> +
>> +	writew(0x11, priv->base + 0x81c);
>> +	writew(0x0, priv->base + 0x81a);
>> +	writew(0x1, priv->base + 0x802);
>> +
>> +	usleep_range(10000, 20000);
>> +
>> +	writew(0x0, priv->base + 0x802);
>> +	writew(0x1, priv->base + 0x2a);
>> +	writew(0x1, priv->base + 0x81a);
> 
> Use macros for these magic numbers.

Please see the comment above, this is some magic register poking init code.

>> +
>> +	usleep_range(10000, 20000);
>> +
>> +	return 0;
>> +}
>> +
>> +static int dwc3_rcar_gen5_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *subnode;
>> +	struct reset_control *reset;
>> +	const char *maximum_speed;
>> +	struct dwc3_rcar_gen5_priv *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(priv->base))
>> +		return dev_err_probe(dev, PTR_ERR(priv->base), "Failed to map registers\n");
>> +
>> +	reset = devm_reset_control_get(dev, NULL);
> 
> Where is the "reset" being used? How is this reset asserted/deasserted?

This is going to be fixed in V2 using 
devm_reset_control_get_exclusive_deasserted().

>> +	if (IS_ERR(reset))
>> +		return dev_err_probe(dev, PTR_ERR(reset), "Failed to get reset control\n");
>> +
>> +	priv->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->clk))
>> +		return dev_err_probe(dev, PTR_ERR(priv->clk), "Failed to get clock control\n");
>> +
>> +	subnode = of_get_compatible_child(dev->of_node, "synopsys,dwc3");
>> +	if (!subnode)
>> +		return dev_err_probe(dev, -ENODEV, "Failed to find DWC3 subnode node\n");
>> +
>> +	ret = of_property_read_string(subnode, "maximum-speed", &maximum_speed);
>> +	of_node_put(subnode);
>> +	if (ret)
>> +		return dev_err_probe(dev, -ENODEV, "Failed to determine maximum speed\n");
> 
> Why is maximum-speed a hard requirement? It should be optional.

Because we need to know whether or not the magic init code (please see 
dwc3_rcar_gen5_init() if (priv->use_usb3_flow) above) should be 
programmed into the glue registers.

>> +	priv->use_usb3_flow = !strcmp(maximum_speed, "super-speed-plus") ||
>> +			      !strcmp(maximum_speed, "super-speed");
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> 
> Do you really want this to be a hard error?

I think so -- why would it be beneficial if this wasn't an error ?

[...]

> Please use the new flatten glue model. The logic for this glue looks
> simple.

It is very simple -- enable clock, power domain, deassert reset, program 
the magic into glue registers and it is done and ready.

> Can you review and evaluate if we can enhance and whether we
> should use the dwc3-generic-plat glue driver for this?

Let me give this a try, I think this should be doable. Thanks !

-- 
Best regards,
Marek Vasut