Re: [PATCH v3 5/9] PCI: rzg3s-host: Add Initial PCIe Host Driver for Renesas RZ/G3S SoC

Geert Uytterhoeven
2025-09-01T17:39:54Z

Hi Mani,

On Mon, 1 Sept 2025 at 16:04, Manivannan Sadhasivam <mani@kernel.org> wrote:
> On Mon, Sep 01, 2025 at 11:25:30AM GMT, Geert Uytterhoeven wrote:
> > On Sun, 31 Aug 2025 at 06:07, Manivannan Sadhasivam <mani@kernel.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@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>
> >
> > > > >> + 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.

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.

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

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

pm_runtime_set_active() may be sufficient, but I am not 100% sure.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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