| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMdYzYoNdAvsO4R5qPJpO4vpd8V0tmsgWhHtXmnM4LR5s=zZpA@mail.gmail.com>
Date: Thu, 16 Jan 2025 08:14:22 -0500
From: Peter Geis <pgwipeout@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Heiko Stuebner <heiko@...ech.de>, 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 Thu, Jan 16, 2025 at 7:59 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> 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.
This is a remnant of the debugging era, thank you for catching it.
>
> > + 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.
The compatibles are remnants from the old method where the driver
bound each device individually. Now that I've switched to the generic
phy system from the usb-phy system, everything is bound at once, they
are no longer necessary.
>
> > + 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.
Thank you for reminding me of this. The error handling portion is
about the only part that remains from the original driver, back before
dev_err_probe became a big thing.
>
> > + 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.
Thank you for catching the remaining debugging stuff I missed stripping out.
>
> Best regards,
> Krzysztof
I appreciate your review.
Very Respectfully,
Peter Geis
Powered by blists - more mailing lists