Hi, Manivannan,

On 9/1/25 18:54, Manivannan Sadhasivam wrote:

On Mon, Sep 01, 2025 at 04:22:16PM GMT, Geert Uytterhoeven wrote:

quoted

Hi Mani,

On Mon, 1 Sept 2025 at 16:04, Manivannan Sadhasivam [off-list ref] wrote:

quoted

On Mon, Sep 01, 2025 at 11:25:30AM GMT, Geert Uytterhoeven wrote:

quoted

On Sun, 31 Aug 2025 at 06:07, Manivannan Sadhasivam [off-list ref] wrote:

quoted

On Sat, Aug 30, 2025 at 02:22:45PM GMT, Claudiu Beznea wrote:

quoted

On 30.08.2025 09:59, Manivannan Sadhasivam wrote:

quoted

On Fri, Jul 04, 2025 at 07:14:05PM GMT, Claudiu wrote:

quoted

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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@sang-engineering.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

quoted

quoted

quoted

quoted

+  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.

quoted

quoted

    pm_runtime_set_active()
    pm_runtime_no_callbacks()
    devm_pm_runtime_enable()

quoted

quoted

quoted

quoted

+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.

Hmm, my understanding was that we need to explicitly handle clocks from the
consumer drivers. But that maybe because, the client drivers I've dealt with
requires configuring the clocks (like setting the rate, re-parenting etc...) on
their own. But if there is no such requirement, then I guess it is OK to rely on
the PM core and clock controller drivers.

When you need to know the actual clock rate, or change it, you
indeed have to handle the clock explicitly.  But it still may be enabled
automatically through the clock domain.

Yeah!

quoted

quoted

quoted

quoted

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().

Two things to note from your statement:

1. 'child device that uses runtime PM' - Not all child drivers are doing
runtime PM on their own. So there is no need to do pm_runtime_resume_and_get()
unless they depend on the parent for resource enablement as below.

It indeed depends on the child device, and on the bus.  For e.g. an
Ethernet controller connected to a simple SoC expansion bus, the bus must
be powered and clock, which is what "simple-pm-bus" takes care of
("simple-bus" does not).

Right. But most of the PCI controller drivers call pm_runtime_resume_and_get()
for no good reasons. They might have just copied the code from a driver that did
it on purpose. So I tend to scrutinize these calls whenever they get added for a
driver.

To be sure I will prepare the next version with something that was
requested: are you OK with keeping pm_runtime_resume_and_get() and add a
comment for it?

Thank you,
Claudiu