Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Stephen,

On Thu, Sep 30, 2021 at 8:16 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Geert Uytterhoeven (2021-09-30 01:01:24)
> > On Thu, Sep 30, 2021 at 7:30 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > > +linux-clk as I don't regularly read my inbox :/
> > >
> > > Quoting marek.vasut@gmail.com (2021-09-07 07:45:12)
> > > > From: Marek Vasut <marek.vasut+renesas@gmail.com>
> > > >
> > > > Add COMMON_CLK dependency, otherwise the following build error occurs:
> > > >   arm-linux-gnueabi-ld: drivers/pci/controller/pcie-rcar-host.o: in function `rcar_pcie_aarch32_abort_handler':
> > > >   pcie-rcar-host.c:(.text+0xdd0): undefined reference to `__clk_is_enabled'
> > > > This should be OK, since all platforms shipping this controller also
> > > > need COMMON_CLK enabled for their clock driver.
> > > >
> > > > Fixes: a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook")
> > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > > Cc: Wolfram Sang <wsa@the-dreams.de>
> > > > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Cc: linux-renesas-soc@vger.kernel.org
> > > > ---
> > > > +CC Stephen, please double-check whether this is the right approach or
> > > >     whether there is some better option
> > >
> > > Stop using __clk_is_enabled()? I don't quite understand what's going on in
> > > the code but __clk_is_enabled() should really go away. I thought we were
> > > close to doing that but now I see a handful of calls have come up. The
> > > API should be replaced by clk_hw_is_enabled() and then removed. We move
> > > it to clk_hw API so that only clk providers can look at it.
> >
> > But this is not a clk provider...
> >
> > > Sigh!
> >
> > ;-)
>
> Exactly!
>
> >
> > > Anyway, fixing the dependency is "ok" but really the long term fix would
> > > be to not use a "is this clk enabled" sort of API. If I'm reading the
> > > code correctly, this is some sort of fault handler that's trying to
> > > avoid hanging the bus while handling the fault so it tries to make sure
> > > the clk is enabled first? Is it a problem if the clk is not actually
> > > enabled here? Would runtime PM enable state of the device work just as
> > > well?
> >
> > Thanks, checking Runtime PM state is a good suggestion. Doing so
> > would require caching a pointer to the PCIe struct device instead of
> > the struct clk.
> > However, pcie_bus_clk is not the module clock, which is managed by
> > Runtime PM, but the PCIe bus clock, which is managed explicitly by
> > the driver.
> > However, I believe that we are checking the wrong clock, as register
> > access needs the module clock to be enabled, not the PCIe bus clock?
> > As the driver just calls pm_runtime_get_sync() and clk_prepare_enable()
> > in .probe(), and never touches Runtime PM status or the PCIe bus clock
> > during the further lifetime of the driver (it cannot be unloaded), both
> > the module clock and the PCIe bus clock should always[*] be enabled
> > when the static copy of the remapped PCIe controller address is valid.
> > [*] Modulo system-wide power transitions like s2ram. Does
> >     pm_runtime_suspended() reflect that state, too?
> >
>
> Great! If that's all correct then simply removing the call to
> __clk_is_enabled() should work. Can we do that?

We first have to double-check that pm_runtime_suspended() reflects
the state, as the reason behind the fault handler is to fix lock-ups
during system-wide power transitions.

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