Re: [RFC PATCH v1 3/6] phy: rockchip: add driver for rk3328 usb3 phy

Krzysztof Kozlowski
2025-01-16T13:01:10Z

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@gmail.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