RE: [PATCH v4 2/3] reset: Add USB2PHY port reset driver for Renesas RZ/V2H(P)

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

Hi Alok,

Thanks for your email.

> From: ALOK TIWARI <alok.a.tiwari@oracle.com>
> Sent: 14 April 2025 14:46
> Subject: Re: [PATCH v4 2/3] reset: Add USB2PHY port reset driver for Renesas RZ/V2H(P)
> 
> 
> > +static int rzv2h_usbphy_reset_assert(struct reset_controller_dev *rcdev,
> > +				     unsigned long id)
> > +{
> > +	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
> > +	struct device *dev = priv->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> > +	if (ret) {
> 
> nit: it will good if we check similar to reset-rzg2l-usbphy-ctrl.c
> pm_runtime_resume_and_get -> 0 on success, or a negative error code
> otherwise.
> 1 → if the device was resumed and incremented usage count
> 0 → if the device was already active or successfully resumed
> if (ret < 0)

No.

As you can see from:
https://github.com/torvalds/linux/blob/master/include/linux/pm_runtime.h#L444

pm_runtime_resume_and_get returns a negative error code or 0 (when
successful).

The same explanation applies to your other comments.


Kind regards,
Fab

> 
> > +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +		return ret;
> > +	}
> > +
> > +	rzv2h_usbphy_assert_helper(priv);
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzv2h_usbphy_reset_deassert(struct reset_controller_dev *rcdev,
> > +				       unsigned long id)
> > +{
> > +	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
> > +	const struct rzv2h_usb2phy_reset_of_data *data = priv->data;
> > +	struct device *dev = priv->dev;
> > +	int ret;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> 
> pm_runtime_resume_and_get -> 0 on success, or a negative error code
> otherwise.
> if (ret < 0)
> 
> > +	if (ret) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +		return ret;
> > +	}
> > +
> > +	scoped_guard(spinlock, &priv->lock) {
> > +		writel(data->reset_deassert_val, priv->base + data->reset_reg);
> > +		writel(data->reset2_release_val, priv->base + data->reset2_reg);
> > +		writel(data->reset_release_val, priv->base + data->reset_reg);
> > +	}
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rzv2h_usbphy_reset_status(struct reset_controller_dev *rcdev,
> > +				     unsigned long id)
> > +{
> > +	struct rzv2h_usb2phy_reset_priv *priv = rzv2h_usbphy_rcdev_to_priv(rcdev);
> > +	struct device *dev = priv->dev;
> > +	int ret;
> > +	u32 reg;
> > +
> > +	ret = pm_runtime_resume_and_get(dev);
> 
> pm_runtime_resume_and_get -> 0 on success, or a negative error code
> otherwise.
> if (ret < 0)
> 
> > +	if (ret) {
> > +		dev_err(dev, "pm_runtime_resume_and_get failed\n");
> > +		return ret;
> > +	}
> > +
> > +	reg = readl(priv->base + priv->data->reset_reg);
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	return (reg & priv->data->reset_status_bits) == priv->data->reset_status_bits;
> > +}
> > +
> > +static const struct reset_control_ops rzv2h_usbphy_reset_ops = {
> > +	.assert = rzv2h_usbphy_reset_assert,
> > +	.deassert = rzv2h_usbphy_reset_deassert,
> > +	.status = rzv2h_usbphy_reset_status,
> > +};
> > +
> > +static int rzv2h_usb2phy_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_reset_probe(struct platform_device *pdev)
> > +{
> > +	const struct rzv2h_usb2phy_reset_of_data *data;
> > +	struct rzv2h_usb2phy_reset_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);
> 
> nit: it will good if we check similar to reset-rzg2l-usbphy-ctrl.c
> pm_runtime_resume_and_get -> 0 on success, or a negative error code
> otherwise.
> if (error < 0)
> 
> > +	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);
> > +
> > +	/* keep usb2phy in asserted state */
> > +	rzv2h_usbphy_assert_helper(priv);
> > +
> > +	pm_runtime_put(dev);
> > +
> > +	priv->rcdev.ops = &rzv2h_usbphy_reset_ops;
> > +	priv->rcdev.of_reset_n_cells = 0;
> > +	priv->rcdev.nr_resets = 1;
> > +	priv->rcdev.of_xlate = rzv2h_usb2phy_reset_of_xlate;
> > +	priv->rcdev.of_node = dev->of_node;
> > +	priv->rcdev.dev = dev;
> > +
> > +	return devm_reset_controller_register(dev, &priv->rcdev);
> > +}
> > +
> 
> 
> Thanks
> Alok