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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Geert Uytterhoeven (2021-09-30 01:01:24)
> 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!
> 
> ;-)

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?




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux