Re: [PATCH v2 2/2] reset: Add USB2PHY control driver for Renesas RZ/V2H(P)

[Lad, Prabhakar]

Hi Philipp,

Thank you for the review.

On Thu, Mar 13, 2025 at 8:37 AM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
>
> 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.
>
I will add a comment for it.

> > +};
> > +
> > +#define rcdev_to_priv(x) container_of(x, struct rzv2h_usb2phy_ctrl_priv, rcdev)
>
> I'd prefer this to be an inline function.
>
OK, I'll add a rzv2h_usbphy_rcdev_to_priv() 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?
>
Based on the HW manual there isn't one.

> > +             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?
>
Mostly the consumers wouldn't call the assert operation in an atomic
context, so usleep_range() could be used here.

Cheers,
Prabhakar