Message-ID: <87001334-5a37-4fba-b08c-3679fb556f83@kernel.org>
Date: Thu, 16 Jan 2025 13:59:38 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Peter Geis <pgwipeout@...il.com>, Heiko Stuebner <heiko@...ech.de>
Cc: zyw@...k-chips.com, kever.yang@...k-chips.com, frank.wang@...k-chips.com,
 william.wu@...k-chips.com, wulf@...k-chips.com,
 linux-rockchip@...ts.infradead.org, Algea Cao <algea.cao@...k-chips.com>,
 Arnd Bergmann <arnd@...db.de>,
 Cristian Ciocaltea <cristian.ciocaltea@...labora.com>,
 Kishon Vijay Abraham I <kishon@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Vinod Koul <vkoul@...nel.org>, Zhang Yubing <yubing.zhang@...k-chips.com>,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org
Subject: Re: [RFC PATCH v1 3/6] phy: rockchip: add driver for rk3328 usb3 phy

On 15/01/2025 02:26, Peter Geis wrote:
> The rk3328 has a combined usb2 and usb3 phy block for the usb3 dwc
> interface. The implementation up until now has been bugged, with
> multiple issues ranging from disconnect detection failures to low
> performance. This driver fixes the majority of the original issues and
> allows better performance for the rk3328 usb3 port.
> 
> This driver sources data from multiple sources, including the rk3328
> trm, the rk3228h trm, emails from Rockchip, and both the 4.4 and 5.10
> downstream drivers. The current implementation allows for basic bring up
> of the phy block and fixes the detection issues. Interrupts are enabled,
> but currently only used for debugging. Features missing currently are
> power management, OTG handling, board specific tuning, regulator control,
> 
> Currently the only known bugs are a AX88179 usb3 gigabit adapter crashes
> when operating at usb3 speeds and transmitting large amounts of data and
> full disconnection detections may be slower than expected (~5-10 seconds).
> 
> Signed-off-by: Peter Geis <pgwipeout@...il.com>
> ---


...

> +}
> +
> +static int rockchip_usb3phy_parse_dt(struct rockchip_usb3phy *usb3phy, struct device *dev)
> +{
> +	int ret;
> +
> +	usb3phy->clk_pipe = devm_clk_get(dev, "usb3phy-pipe");
> +	if (IS_ERR(usb3phy->clk_pipe)) {
> +		dev_err(dev, "could not get usb3phy pipe clock\n");

Syntax is always:
return dev_err_probe

> +		return PTR_ERR(usb3phy->clk_pipe);
> +	}
> +
> +	usb3phy->clk_otg = devm_clk_get(dev, "usb3phy-otg");
> +	if (IS_ERR(usb3phy->clk_otg)) {
> +		dev_err(dev, "could not get usb3phy otg clock\n");
> +		return PTR_ERR(usb3phy->clk_otg);
> +	}
> +
> +	usb3phy->clk_ref = devm_clk_get(dev, "refclk-usb3otg");
> +	if (IS_ERR(usb3phy->clk_ref)) {
> +		dev_err(dev, "could not get usb3phy ref clock\n");
> +		return PTR_ERR(usb3phy->clk_ref);
> +	}
> +
> +	usb3phy->u2por_rst = devm_reset_control_get(dev, "usb3phy-u2-por");
> +	if (IS_ERR(usb3phy->u2por_rst)) {
> +		dev_err(dev, "no usb3phy-u2-por reset control found\n");
> +		return PTR_ERR(usb3phy->u2por_rst);
> +	}
> +
> +	usb3phy->u3por_rst = devm_reset_control_get(dev, "usb3phy-u3-por");
> +	if (IS_ERR(usb3phy->u3por_rst)) {
> +		dev_err(dev, "no usb3phy-u3-por reset control found\n");
> +		return PTR_ERR(usb3phy->u3por_rst);
> +	}
> +
> +	usb3phy->pipe_rst = devm_reset_control_get(dev, "usb3phy-pipe-mac");
> +	if (IS_ERR(usb3phy->pipe_rst)) {
> +		dev_err(dev, "no usb3phy_pipe_mac reset control found\n");
> +		return PTR_ERR(usb3phy->pipe_rst);
> +	}
> +
> +	usb3phy->utmi_rst = devm_reset_control_get(dev, "usb3phy-utmi-mac");
> +	if (IS_ERR(usb3phy->utmi_rst)) {
> +		dev_err(dev, "no usb3phy-utmi-mac reset control found\n");
> +		return PTR_ERR(usb3phy->utmi_rst);
> +	}
> +
> +	usb3phy->pipe_apb_rst = devm_reset_control_get(dev, "usb3phy-pipe-apb");
> +	if (IS_ERR(usb3phy->pipe_apb_rst)) {
> +		dev_err(dev, "no usb3phy-pipe-apb reset control found\n");
> +		return PTR_ERR(usb3phy->pipe_apb_rst);
> +	}
> +
> +	usb3phy->utmi_apb_rst = devm_reset_control_get(dev, "usb3phy-utmi-apb");
> +	if (IS_ERR(usb3phy->utmi_apb_rst)) {
> +		dev_err(dev, "no usb3phy-utmi-apb reset control found\n");
> +		return PTR_ERR(usb3phy->utmi_apb_rst);
> +	}
> +
> +	usb3phy->ls_irq = of_irq_get_byname(dev->of_node, "linestate");
> +	if (usb3phy->ls_irq < 0) {
> +		dev_err(dev, "no linestate irq provided\n");
> +		return usb3phy->ls_irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, usb3phy->ls_irq, NULL, rockchip_usb3phy_linestate_irq,
> +					IRQF_ONESHOT, "rockchip_usb3phy_ls", usb3phy);
> +	if (ret) {
> +		dev_err(dev, "failed to request linestate irq handle\n");
> +		return ret;
> +	}
> +
> +	usb3phy->bvalid_irq = of_irq_get_byname(dev->of_node, "bvalid");
> +	if (usb3phy->bvalid_irq < 0) {
> +		dev_err(dev, "no bvalid irq provided\n");
> +		return usb3phy->bvalid_irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, usb3phy->bvalid_irq, NULL, rockchip_usb3phy_bvalid_irq,
> +					IRQF_ONESHOT, "rockchip_usb3phy_bvalid", usb3phy);
> +	if (ret) {
> +		dev_err(dev, "failed to request bvalid irq handle\n");
> +		return ret;
> +	}
> +
> +	usb3phy->id_irq = of_irq_get_byname(dev->of_node, "id");
> +	if (usb3phy->id_irq < 0) {
> +		dev_err(dev, "no id irq provided\n");
> +		return usb3phy->id_irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, usb3phy->id_irq, NULL, rockchip_usb3phy_id_irq,
> +					IRQF_ONESHOT, "rockchip_usb3phy-id", usb3phy);
> +	if (ret) {
> +		dev_err(dev, "failed to request id irq handle\n");
> +		return ret;
> +	}
> +
> +		usb3phy->rxdet_irq = of_irq_get_byname(dev->of_node, "rxdet");
> +	if (usb3phy->rxdet_irq < 0) {
> +		dev_err(dev, "no rxdet irq provided\n");
> +		return usb3phy->rxdet_irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, usb3phy->rxdet_irq, NULL, rockchip_usb3phy_rxdet_irq,
> +					IRQF_ONESHOT, "rockchip_usb3phy-rxdet", usb3phy);
> +	if (ret) {
> +		dev_err(dev, "failed to request rxdet irq handle\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rockchip_usb3phy_exit(struct phy *phy)
> +{
> +	struct rockchip_usb3phy_port *port = phy_get_drvdata(phy);
> +	struct rockchip_usb3phy *usb3phy = dev_get_drvdata(phy->dev.parent);
> +
> +	dev_dbg(usb3phy->dev, "usb3phy_shutdown\n");
> +	rockchip_usb3phy_reset(usb3phy, true, port->type);
> +
> +	return 0;
> +}
> +
> +static const struct phy_ops rockchip_usb3phy_ops = {
> +	.init		= rockchip_usb3phy_init,
> +	.exit		= rockchip_usb3phy_exit,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct regmap_config rockchip_usb3phy_utmi_port_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x400,
> +	.cache_type = REGCACHE_NONE,
> +	.fast_io = true,
> +	.name = "utmi-port",
> +};
> +
> +static const struct regmap_config rockchip_usb3phy_pipe_port_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x400,
> +	.cache_type = REGCACHE_NONE,
> +	.fast_io = true,
> +	.name = "pipe-port",
> +};
> +
> +static const struct regmap_config rockchip_usb3phy_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x400,
> +	.write_flag_mask = REG_WRITE_MASK,
> +	.cache_type = REGCACHE_NONE,
> +	.fast_io = true,
> +};
> +
> +static int rockchip_usb3phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *child_np;
> +	struct phy_provider *provider;
> +	struct rockchip_usb3phy *usb3phy;
> +	const struct regmap_config regmap_config = rockchip_usb3phy_regmap_config;
> +	void __iomem *base;
> +	int i, ret;
> +
> +	dev_dbg(dev, "Probe usb3phy main block\n");
> +	usb3phy = devm_kzalloc(dev, sizeof(*usb3phy), GFP_KERNEL);
> +	if (!usb3phy)
> +		return -ENOMEM;
> +
> +	ret = rockchip_usb3phy_parse_dt(usb3phy, dev);
> +	if (ret) {
> +		dev_err(dev, "parse dt failed %i\n", ret);

return dev_err_probe()

And do not print errors twice.

> +		return ret;




> +
> +	/* take apb interface out of reset */
> +	reset_control_deassert(usb3phy->utmi_apb_rst);
> +	reset_control_deassert(usb3phy->pipe_apb_rst);
> +
> +	/* enable usb3phy rx detection to fix disconnection issues */
> +	regmap_write(usb3phy->regmap, USB3PHY_WAKEUP_CON_REG,
> +		     (USB3_RXDET_EN | (USB3_RXDET_EN << REG_WRITE_SHIFT)));
> +
> +	dev_dbg(dev, "Completed usb3phy core probe\n");

Drop.

> +
> +	/* probe the actual ports */
> +	i = 0;
> +	for_each_available_child_of_node(np, child_np) {
> +		const struct regmap_config *regmap_port_config;
> +		struct rockchip_usb3phy_port *port = &usb3phy->ports[i];
> +		struct phy *phy;
> +
> +		if (of_node_name_eq(child_np, "utmi-port")) {

Why are you comparing node names, if you decided to put there compatibles?

Decide on one method, but wait for full DT review.

> +			port->type = USB3PHY_TYPE_USB2;
> +			regmap_port_config = &rockchip_usb3phy_utmi_port_regmap_config;
> +		} else if (of_node_name_eq(child_np, "pipe-port")) {
> +			port->type	= USB3PHY_TYPE_USB3;
> +			regmap_port_config = &rockchip_usb3phy_pipe_port_regmap_config;
> +		} else {
> +			dev_err(dev, "unknown child node port type %s\n", child_np->name);
> +			goto err_port;
> +		}
> +
> +		base = devm_of_iomap(dev, child_np, 0, NULL);
> +		if (IS_ERR(base)) {
> +			dev_err(dev, "failed port ioremap\n");
> +			ret = PTR_ERR(base);
> +			goto err_port;
> +		}
> +
> +		port->regmap = devm_regmap_init_mmio(dev, base, regmap_port_config);
> +		if (IS_ERR(port->regmap)) {
> +			dev_err(dev, "regmap init failed\n");
> +			ret = PTR_ERR(port->regmap);
> +			goto err_port;
> +		}
> +
> +		phy = devm_phy_create(dev, child_np, &rockchip_usb3phy_ops);
> +		if (IS_ERR(phy)) {
> +			dev_err_probe(dev, PTR_ERR(phy), "failed to create phy\n");
> +			ret = PTR_ERR(phy);

Just read how dev_err_probe() works. The syntax is:
ret = dev_err_probe.

> +			goto err_port;
> +		}
> +
> +		port->phy = phy;
> +		phy_set_drvdata(port->phy, port);
> +
> +		/* to prevent out of boundary */
> +		if (++i >= USB3PHY_TYPE_MAX) {
> +			of_node_put(child_np);
> +			break;
> +		}
> +
> +		dev_info(dev, "Completed usb3phy %s port init\n", child_np->name);
Drop, not really helping.

Best regards,
Krzysztof

Powered by blists - more mailing lists