Hi Mani, On Sun, 31 Aug 2025 at 06:07, Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote: > On Sat, Aug 30, 2025 at 02:22:45PM GMT, Claudiu Beznea wrote: > > On 30.08.2025 09:59, Manivannan Sadhasivam wrote: > > > On Fri, Jul 04, 2025 at 07:14:05PM GMT, Claudiu wrote: > > >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > > >> > > >> The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express > > >> Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions > > >> only as a root complex, with a single-lane (x1) configuration. The > > >> controller includes Type 1 configuration registers, as well as IP > > >> specific registers (called AXI registers) required for various adjustments. > > >> > > >> Hardware manual can be downloaded from the address in the "Link" section. > > >> The following steps should be followed to access the manual: > > >> 1/ Click the "User Manual" button > > >> 2/ Click "Confirm"; this will start downloading an archive > > >> 3/ Open the downloaded archive > > >> 4/ Navigate to r01uh1014ej*-rzg3s-users-manual-hardware -> Deliverables > > >> 5/ Open the file r01uh1014ej*-rzg3s.pdf > > >> > > >> Link: https://www.renesas.com/en/products/rz-g3s?queryID=695cc067c2d89e3f271d43656ede4d12 > > >> Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx> > > >> + ret = pm_runtime_resume_and_get(dev); > > >> + if (ret) > > >> + return ret; > > >> + > > > > > > Do you really need to do resume_and_get()? If not, you should do: > > > > It it's needed to enable the clock PM domain the device is part of. > > > > I've replied below. > > > > > > > pm_runtime_set_active() > > > pm_runtime_no_callbacks() > > > devm_pm_runtime_enable() > > >> +static int rzg3s_pcie_suspend_noirq(struct device *dev) > > >> +{ > > >> + struct rzg3s_pcie_host *host = dev_get_drvdata(dev); > > >> + const struct rzg3s_pcie_soc_data *data = host->data; > > >> + struct regmap *sysc = host->sysc; > > >> + int ret; > > >> + > > >> + ret = pm_runtime_put_sync(dev); > > >> + if (ret) > > >> + return ret; > > > > > > Since there are no runtime callbacks present, managing runtime PM in the driver > > > makes no sense. > > > > The PCIe device is part of a clock power domain. Dropping > > pm_runtime_enable()/pm_runtime_put_sync() in this driver will lead to this > > IP failing to work as its clocks will not be enabled/disabled. If you don't > > like the pm_runtime_* approach that could be replaced with: > > > > devm_clk_get_enabled() in probe and clk_disable()/clk_enable() on > > suspend/resume. W/o clocks the IP can't work. > > Yes, you should explicitly handle clocks in the driver. Runtime PM makes sense > if you have a power domain attached to the IP, which you also do as I see now. > So to conclude, you should enable/disable the clocks explicitly for managing > clocks and use runtime PM APIs for managing the power domain associated with > clock controller. Why? For the past decade, we've been trying to get rid of explicit module clock handling for all devices that are always part of a clock domain. The Linux PM Domain abstraction is meant for both power and clock domains. This is especially useful when a device is present on multiple SoCs, on some also part of a power domain, and the number of module clocks that needs to be enabled for it to function is not the same on all SoCs. In such cases, the PM Domain abstraction takes care of many of the integration-specific differences. > But please add a comment above pm_runtime_resume_and_get() to make it clear as > most of the controller drivers are calling it for no reason. Note that any child device that uses Runtime PM depends on all its parents in the hierarchy to call pm_runtime_enable() and pm_runtime_resume_and_get(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
