Hi Philipp,
Thanks for your feedback!
> From: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Sent: 13 March 2025 08:37
> Subject: Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)
>
> On Mi, 2025-03-05 at 12:39 +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add support for the USB2PHY control driver on the Renesas RZ/V2H(P) SoC.
> > Make the driver handle reset and power-down operations for the USB2PHY.
> >
> > Pass OF data to support future SoCs with similar USB2PHY hardware but
> > different register configurations. Define device-specific initialization
> > values and control register settings in OF data to ensure flexibility
> > for upcoming SoCs.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > drivers/reset/Kconfig | 7 +
> > drivers/reset/Makefile | 1 +
> > drivers/reset/reset-rzv2h-usb2phy-ctrl.c | 223 +++++++++++++++++++++++
> > 3 files changed, 231 insertions(+)
> > create mode 100644 drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 5b3abb6db248..bac08dae8905 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -218,6 +218,13 @@ config RESET_RZG2L_USBPHY_CTRL
> > Support for USBPHY Control found on RZ/G2L family. It mainly
> > controls reset and power down of the USB/PHY.
> >
> > +config RESET_RZV2H_USB2PHY_CTRL
> > + tristate "Renesas RZ/V2H(P) (and similar SoCs) USB2PHY control driver"
> > + depends on ARCH_RENESAS || COMPILE_TEST
> > + help
> > + Support for USB2PHY Control found on the RZ/V2H(P) SoC (and similar SoCs).
> > + It mainly controls reset and power down of the USB2 PHY.
> > +
> > config RESET_SCMI
> > tristate "Reset driver controlled via ARM SCMI interface"
> > depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index 677c4d1e2632..3cb3df018cf8 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_RESET_QCOM_AOSS) += reset-qcom-aoss.o
> > obj-$(CONFIG_RESET_QCOM_PDC) += reset-qcom-pdc.o
> > obj-$(CONFIG_RESET_RASPBERRYPI) += reset-raspberrypi.o
> > obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
> > +obj-$(CONFIG_RESET_RZV2H_USB2PHY_CTRL) += reset-rzv2h-usb2phy-ctrl.o
> > obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
> > obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
> > obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
> > diff --git a/drivers/reset/reset-rzv2h-usb2phy-ctrl.c b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > new file mode 100644
> > index 000000000000..a6daeaf37e1c
> > --- /dev/null
> > +++ b/drivers/reset/reset-rzv2h-usb2phy-ctrl.c
> > @@ -0,0 +1,223 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/V2H(P) USB2PHY control driver
> > + *
> > + * Copyright (C) 2025 Renesas Electronics Corporation
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +#include <linux/reset-controller.h>
> > +
> > +struct rzv2h_usb2phy_regval {
> > + u16 reg;
> > + u16 val;
> > +};
> > +
> > +struct rzv2h_usb2phy_data {
> > + const struct rzv2h_usb2phy_regval *init_vals;
> > + unsigned int init_val_count;
> > +
> > + u16 ctrl_reg;
> > + u16 ctrl_assert_val;
> > + u16 ctrl_deassert_val;
> > + u16 ctrl_status_bits;
> > + u16 ctrl_release_val;
> > +
> > + u16 ctrl2_reg;
> > + u16 ctrl2_acquire_val;
> > + u16 ctrl2_release_val;
> > +};
> > +
> > +struct rzv2h_usb2phy_ctrl_priv {
> > + const struct rzv2h_usb2phy_data *data;
> > + void __iomem *base;
> > + struct device *dev;
> > + struct reset_controller_dev rcdev;
> > + spinlock_t lock;
>
> Lock without comment.
>
> > +};
> > +
> > +#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)
>
> I'd prefer this to be an inline function.
>
> > +
> > +static int rzv2h_usbphy_ctrl_assert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > + const struct rzv2h_usb2phy_data *data = priv->data;
> > + struct device *dev = priv->dev;
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret) {
> > + dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > + return ret;
> > + }
> > + scoped_guard(spinlock, &priv->lock) {
> > + writel(data->ctrl2_acquire_val, priv->base + data->ctrl2_reg);
>
> Comparing to deassert, I wonder why is there no ctrl_acquire_val?
>
> > + writel(data->ctrl_assert_val, priv->base + data->ctrl_reg);
> > + }
> > +
> > + /* The reset line needs to be asserted for more than 10 microseconds. */
> > + udelay(11);
>
> Could this be usleep_range() instead?
>
> > + pm_runtime_put(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2h_usbphy_ctrl_deassert(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > + const struct rzv2h_usb2phy_data *data = priv->data;
> > + struct device *dev = priv->dev;
> > + int ret;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret) {
> > + dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > + return ret;
> > + }
> > +
> > + scoped_guard(spinlock, &priv->lock) {
> > + writel(data->ctrl_deassert_val, priv->base + data->ctrl_reg);
> > + writel(data->ctrl2_release_val, priv->base + data->ctrl2_reg);
> > + writel(data->ctrl_release_val, priv->base + data->ctrl_reg);
> > + }
> > +
> > + pm_runtime_put(dev);
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2h_usbphy_ctrl_status(struct reset_controller_dev *rcdev,
> > + unsigned long id)
> > +{
> > + struct rzv2h_usb2phy_ctrl_priv *priv = rcdev_to_priv(rcdev);
> > + struct device *dev = priv->dev;
> > + int ret;
> > + u32 reg;
> > +
> > + ret = pm_runtime_resume_and_get(dev);
> > + if (ret) {
> > + dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > + return ret;
> > + }
> > +
> > + scoped_guard(spinlock, &priv->lock)
> > + reg = readl(priv->base + priv->data->ctrl_reg);
> > +
> > + pm_runtime_put(dev);
> > +
> > + return (reg & priv->data->ctrl_status_bits) == priv->data->ctrl_status_bits;
> > +}
> > +
> > +static const struct reset_control_ops rzv2h_usbphy_ctrl_reset_ops = {
> > + .assert = rzv2h_usbphy_ctrl_assert,
> > + .deassert = rzv2h_usbphy_ctrl_deassert,
> > + .status = rzv2h_usbphy_ctrl_status,
> > +};
> > +
> > +static int rzv2h_reset_of_xlate(struct reset_controller_dev *rcdev,
> > + const struct of_phandle_args *reset_spec)
> > +{
> > + /* No special handling needed, we have only one reset line per device */
> > + return 0;
> > +}
> > +
> > +static int rzv2h_usb2phy_ctrl_probe(struct platform_device *pdev)
> > +{
> > + const struct rzv2h_usb2phy_data *data;
> > + struct rzv2h_usb2phy_ctrl_priv *priv;
> > + struct device *dev = &pdev->dev;
> > + struct reset_control *rstc;
> > + int error;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + data = of_device_get_match_data(dev);
> > + if (!data)
> > + return dev_err_probe(dev, -ENODEV,
> > + "failed to match device\n");
> > +
> > + priv->data = data;
> > + priv->dev = dev;
> > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->base))
> > + return PTR_ERR(priv->base);
> > +
> > + rstc = devm_reset_control_get_shared_deasserted(dev, NULL);
> > + if (IS_ERR(rstc))
> > + return dev_err_probe(dev, PTR_ERR(rstc),
> > + "failed to get deasserted reset\n");
> > +
> > + spin_lock_init(&priv->lock);
> > + dev_set_drvdata(dev, priv);
> > +
> > + error = devm_pm_runtime_enable(dev);
> > + if (error)
> > + return dev_err_probe(dev, error, "Failed to enable pm_runtime\n");
> > +
> > + error = pm_runtime_resume_and_get(dev);
> > + if (error)
> > + return dev_err_probe(dev, error, "pm_runtime_resume_and_get failed\n");
> > +
> > + for (unsigned int i = 0; i < data->init_val_count; i++)
> > + writel(data->init_vals[i].val, priv->base + data->init_vals[i].reg);
> > +
> > + pm_runtime_put(dev);
> > +
> > + priv->rcdev.ops = &rzv2h_usbphy_ctrl_reset_ops;
> > + priv->rcdev.of_reset_n_cells = 0;
> > + priv->rcdev.nr_resets = 1;
> > + priv->rcdev.of_xlate = rzv2h_reset_of_xlate;
> > + priv->rcdev.of_node = dev->of_node;
> > + priv->rcdev.dev = dev;
> > +
> > + return devm_reset_controller_register(dev, &priv->rcdev);
> > +}
> > +
> > +static const struct rzv2h_usb2phy_regval rzv2h_init_vals[] = {
> > + { .reg = 0xc10, .val = 0x67c },
> > + { .reg = 0xc14, .val = 0x1f },
> > + { .reg = 0x600, .val = 0x909 },
>
> What are these registers and what are those values doing?
Unfortunately, there are some licensing restrictions on this IP, this is
the best that we can do, as per the license agreement.
>
> > +};
> > +
> > +static const struct rzv2h_usb2phy_data rzv2h_of_data = {
> > + .init_vals = rzv2h_init_vals,
> > + .init_val_count = ARRAY_SIZE(rzv2h_init_vals),
> > + .ctrl_reg = 0,
> > + .ctrl_assert_val = 0x206,
> > + .ctrl_status_bits = BIT(2),
> > + .ctrl_deassert_val = 0x200,
> > + .ctrl_release_val = 0x0,
> > + .ctrl2_reg = 0xb04,
> > + .ctrl2_acquire_val = 0x303,
> > + .ctrl2_release_val = 0x3,
>
> This is really opaque. I would have expected some defines for the
> register bits (or fields?) at least.
We have the same limitations for these bits I am afraid.
Cheers,
Fab
>
> > +};
> > +
> > +static const struct of_device_id rzv2h_usb2phy_ctrl_match_table[] = {
> > + { .compatible = "renesas,r9a09g057-usb2phy-ctrl", .data = &rzv2h_of_data },
> > + { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzv2h_usb2phy_ctrl_match_table);
> > +
> > +static struct platform_driver rzv2h_usb2phy_ctrl_driver = {
> > + .driver = {
> > + .name = "rzv2h_usb2phy_ctrl",
> > + .of_match_table = rzv2h_usb2phy_ctrl_match_table,
> > + },
> > + .probe = rzv2h_usb2phy_ctrl_probe,
> > +};
> > +module_platform_driver(rzv2h_usb2phy_ctrl_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Renesas RZ/V2H(P) USB2PHY Control");
>
> regards
> Philipp
