Re: [PATCH 00/16] Add initial USB support for the Renesas RZ/G3S SoC
From: Ulf Hansson <hidden>
Date: 2024-08-31 10:32:40
Also in:
linux-arm-kernel, linux-clk, linux-phy, linux-pm, linux-renesas-soc, linux-usb, lkml
[...]
quoted
If not, there are two other options that can be considered I think. *) Using the genpd on/off notifiers, to really allow the consumer driver of the reset-control to know when the PM domain gets turned on/off. **) Move the entire reset handling into the PM domain provider, as it obviously knows when the domain is getting turned on/off.This option is what I've explored, tested on my side. I explored it in 2 ways: 1/ SYSC modeled as an individual PM domain provider (this is more appropriate to how HW manual described the hardware) with this the PHY reset DT node would have to get 2 PM domains handlers (one for the current PM domain provider and the other one for SYSC): + phyrst: usbphy-ctrl@11e00000 { + compatible = "renesas,r9a08g045-usbphy-ctrl"; + reg = <0 0x11e00000 0 0x10000>; + clocks = <&cpg CPG_MOD R9A08G045_USB_PCLK>; + resets = <&cpg R9A08G045_USB_PRESETN>; + power-domain-names = "cpg", "sysc"; + power-domains = <&cpg R9A08G045_PD_USB_PHY>, <&sysc R9A08G045_SYSC_PD_USB>; + #reset-cells = <1>; + status = "disabled"; + + usb0_vbus_otg: regulator-vbus { + regulator-name = "vbus"; + }; + }; +
According to what you have described earlier/above, modelling the SYSC as a PM domain provider seems like a better description of the HW to me. Although, as I said earlier, if you prefer the reset approach, I would not object to that.
and the PHY reset driver will get bulky with powering on/off both of these,
at least with my current implementation, something like (and the following
code is in probe()):
+ if (priv->set_power) {
+ priv->cpg_genpd_dev = dev_pm_domain_attach_by_name(dev, "cpg");
+ if (IS_ERR(priv->cpg_genpd_dev)) {
+ dev_err_probe(dev, error, "Failed to attach CPG PM
domain!");
+ error = PTR_ERR(priv->cpg_genpd_dev);
+ goto err_pm_runtime_put;
+ }
+
+ priv->sysc_genpd_dev = dev_pm_domain_attach_by_name(dev,
"sysc");
+ if (IS_ERR(priv->sysc_genpd_dev)) {
+ dev_err_probe(dev, error, "Failed to attach sysc PM
domain!");
+ error = PTR_ERR(priv->sysc_genpd_dev);
+ goto err_genpd_cpg_detach;
+ }
+
+ priv->cpg_genpd_dl = device_link_add(dev, priv->cpg_genpd_dev,
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_STATELESS);
+ if (!priv->cpg_genpd_dl) {
+ dev_err_probe(dev, -ENOMEM, "Failed to add CPG
genpd device link!");
+ goto err_genpd_sysc_detach;
+ }
+
+ priv->sysc_genpd_dl = device_link_add(dev,
priv->sysc_genpd_dev,
+ DL_FLAG_PM_RUNTIME |
+ DL_FLAG_STATELESS);
+ if (!priv->sysc_genpd_dl) {
+ dev_err_probe(dev, -ENOMEM, "Failed to add sysc
genpd device link!");
+ goto err_genpd_cpg_dl_del;
+ }
+
+
+ error = pm_runtime_resume_and_get(priv->cpg_genpd_dev);
+ if (error) {
+ dev_err_probe(dev, error, "Failed to runtime resume
cpg PM domain!");
+ goto err_genpd_sysc_dl_del;
+ }
+
+ error = pm_runtime_resume_and_get(priv->sysc_genpd_dev);
+ if (error) {
+ dev_err_probe(dev, error, "Failed to runtime resume
sysc PM domain!");
+ goto err_genpd_cpg_off;
+ }
+ }Indeed, the code above looks bulky. Fortunately, we now have dev|devm_pm_domain_attach_list(), which replaces all of the code above. [...] Kind regards Uffe