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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdUu0uXBJndcwWoZp8NNyBJox5dZw4aoB8Ex50vBDDtP7g@mail.gmail.com>
Date: Mon, 1 Sep 2025 11:25:30 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Manivannan Sadhasivam <mani@...nel.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

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.

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