Hi Stephen,
On Thu, Sep 30, 2021 at 7:30 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> +linux-clk as I don't regularly read my inbox :/
>
> Quoting marek.vasut@xxxxxxxxx (2021-09-07 07:45:12)
> > From: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > Cc: Wolfram Sang <wsa@xxxxxxxxxxxxx>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> > ---
> > +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!
;-)
> 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?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
