Message-ID: <6f2hpdkonomgrfzqoupcex2rpqtlhql4lmsqm7hqk25qakp7ax@bfrzflghmnev>
Date: Mon, 1 Sep 2025 19:33:51 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Claudiu Beznea <claudiu.beznea@...on.dev>, bhelgaas@...gle.com, 
	lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, geert+renesas@...der.be, magnus.damm@...il.com, 
	catalin.marinas@....com, will@...nel.org, mturquette@...libre.com, sboyd@...nel.org, 
	p.zabel@...gutronix.de, lizhi.hou@....com, linux-pci@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH v3 5/9] PCI: rzg3s-host: Add Initial PCIe Host Driver for
 Renesas RZ/G3S SoC

On Mon, Sep 01, 2025 at 11:25:30AM GMT, Geert Uytterhoeven wrote:
> Hi Mani,
> 
> On Sun, 31 Aug 2025 at 06:07, Manivannan Sadhasivam <mani@...nel.org> 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@...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@...g-engineering.com>
> > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> 
> > > >> +  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.
> 

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.

Thanks for clarifying.

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

2. 'child devices depending on parents in the hierarchy' - Again, not all
child drivers require their parent to enable the resources. In those cases, they
can just call pm_runtime_set_active() and pm_runtime_enable() in their probe.
There is absolutely no need to do pm_runtime_resume_and_get() AFAIK (correct me
if I'm wrong).

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists