| Message ID | 20210907144512.5238-1-marek.vasut@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: rcar: Add missing COMMON_CLK dependency | expand |
On Tue, Sep 07, 2021 at 04:45:12PM +0200, marek.vasut@gmail.com wrote: > 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 Hi Stephen, can you please have a look into this please to see if there is a better way of fixing this breakage ? We should aim for one of the upcoming rcX, thank you very much. Lorenzo > --- > drivers/pci/controller/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index 326f7d13024f..ee6f5e525d3a 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > config PCIE_RCAR_HOST > bool "Renesas R-Car PCIe host controller" > depends on ARCH_RENESAS || COMPILE_TEST > + depends on COMMON_CLK > depends on PCI_MSI_IRQ_DOMAIN > help > Say Y here if you want PCIe controller support on R-Car SoCs in host > @@ -74,6 +75,7 @@ config PCIE_RCAR_HOST > config PCIE_RCAR_EP > bool "Renesas R-Car PCIe endpoint controller" > depends on ARCH_RENESAS || COMPILE_TEST > + depends on COMMON_CLK > depends on PCI_ENDPOINT > help > Say Y here if you want PCIe controller support on R-Car SoCs in > -- > 2.33.0 >
Hi Marek, Thanks for your patch! On Tue, Sep 7, 2021 at 4:45 PM <marek.vasut@gmail.com> wrote: > 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 is a link failure for the host driver... > 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 > --- > drivers/pci/controller/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index 326f7d13024f..ee6f5e525d3a 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > config PCIE_RCAR_HOST > bool "Renesas R-Car PCIe host controller" > depends on ARCH_RENESAS || COMPILE_TEST > + depends on COMMON_CLK This part is OK. > depends on PCI_MSI_IRQ_DOMAIN > help > Say Y here if you want PCIe controller support on R-Car SoCs in host > @@ -74,6 +75,7 @@ config PCIE_RCAR_HOST > config PCIE_RCAR_EP > bool "Renesas R-Car PCIe endpoint controller" > depends on ARCH_RENESAS || COMPILE_TEST > + depends on COMMON_CLK ... so why did you add a dependency to the endpoint driver, too? > depends on PCI_ENDPOINT > help > Say Y here if you want PCIe controller support on R-Car SoCs in Gr{oetje,eeting}s, Geert
On 9/21/21 6:08 PM, Geert Uytterhoeven wrote: [...] >> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig >> index 326f7d13024f..ee6f5e525d3a 100644 >> --- a/drivers/pci/controller/Kconfig >> +++ b/drivers/pci/controller/Kconfig >> @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 >> config PCIE_RCAR_HOST >> bool "Renesas R-Car PCIe host controller" >> depends on ARCH_RENESAS || COMPILE_TEST >> + depends on COMMON_CLK > > This part is OK. This part is also identical in the patch from Arnd, so you can just pick that one as a fix and be done with it: [PATCH] PCI: rcar: add COMMON_CLK dependency https://patchwork.kernel.org/project/linux-pci/patch/20210920095730.1216692-1-arnd@kernel.org/
On Wed, Sep 22, 2021 at 01:13:11AM +0200, Marek Vasut wrote: > On 9/21/21 6:08 PM, Geert Uytterhoeven wrote: > > [...] > > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > > > index 326f7d13024f..ee6f5e525d3a 100644 > > > --- a/drivers/pci/controller/Kconfig > > > +++ b/drivers/pci/controller/Kconfig > > > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > > > config PCIE_RCAR_HOST > > > bool "Renesas R-Car PCIe host controller" > > > depends on ARCH_RENESAS || COMPILE_TEST > > > + depends on COMMON_CLK > > > > This part is OK. > > This part is also identical in the patch from Arnd, so you can just pick > that one as a fix and be done with it: > > [PATCH] PCI: rcar: add COMMON_CLK dependency > https://patchwork.kernel.org/project/linux-pci/patch/20210920095730.1216692-1-arnd@kernel.org/ It is not strictly identical (Arnd's patch only touches the COMPILE_TEST option). Bjorn, shall we pick Arnd's patch up then ? We should be fixing this in one of the upcoming -rcs since we introduced it in the last merge window. Thanks, Lorenzo
On Wed, Sep 29, 2021 at 03:55:09PM +0100, Lorenzo Pieralisi wrote: > On Wed, Sep 22, 2021 at 01:13:11AM +0200, Marek Vasut wrote: > > On 9/21/21 6:08 PM, Geert Uytterhoeven wrote: > > > > [...] > > > > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > > > > index 326f7d13024f..ee6f5e525d3a 100644 > > > > --- a/drivers/pci/controller/Kconfig > > > > +++ b/drivers/pci/controller/Kconfig > > > > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > > > > config PCIE_RCAR_HOST > > > > bool "Renesas R-Car PCIe host controller" > > > > depends on ARCH_RENESAS || COMPILE_TEST > > > > + depends on COMMON_CLK > > > > > > This part is OK. > > > > This part is also identical in the patch from Arnd, so you can just pick > > that one as a fix and be done with it: > > > > [PATCH] PCI: rcar: add COMMON_CLK dependency > > https://patchwork.kernel.org/project/linux-pci/patch/20210920095730.1216692-1-arnd@kernel.org/ > > It is not strictly identical (Arnd's patch only touches the COMPILE_TEST > option). > > Bjorn, shall we pick Arnd's patch up then ? We should be fixing this in > one of the upcoming -rcs since we introduced it in the last merge > window. IIUC, a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort hook") appeared in v5.15-rc1 and added a use of __clk_is_enabled(), which is only available when COMMON_CLK=y. PCIE_RCAR_HOST depends on ARCH_RENESAS || COMPILE_TEST. ARCH_RENESAS is an ARM64 platform, so when COMPILE_TEST is not set, I think we get COMMON_CLK=y via this: config ARM64 select COMMON_CLK But when ARCH_RENESAS is not set and COMPILE_TEST=y, there's nothing that enforces the dependency on COMMON_CLK. Personally I like the first hunk of Marek's patch at [1] because the dependency on COMMON_CLK is explicit: config PCIE_RCAR_HOST depends on ARCH_RENESAS || COMPILE_TEST depends on COMMON_CLK Whereas Arnd's patch [2] implicitly depends on the fact that the platform selects COMMON_CLK: config PCIE_RCAR_HOST depends on ARCH_RENESAS || (COMMON_CLK && COMPILE_TEST) But either is fine and I agree we should fix it soonish. Bjorn [1] https://lore.kernel.org/all/20210907144512.5238-1-marek.vasut@gmail.com/ [2] https://lore.kernel.org/all/20210920095730.1216692-1-arnd@kernel.org/
On Wed, Sep 29, 2021 at 6:32 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Sep 29, 2021 at 03:55:09PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Sep 22, 2021 at 01:13:11AM +0200, Marek Vasut wrote: > > > On 9/21/21 6:08 PM, Geert Uytterhoeven wrote: > > > > > > [...] > > > > > > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > > > > > index 326f7d13024f..ee6f5e525d3a 100644 > > > > > --- a/drivers/pci/controller/Kconfig > > > > > +++ b/drivers/pci/controller/Kconfig > > > > > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > > > > > config PCIE_RCAR_HOST > > > > > bool "Renesas R-Car PCIe host controller" > > > > > depends on ARCH_RENESAS || COMPILE_TEST > > > > > + depends on COMMON_CLK > > > > > > > > Bjorn, shall we pick Arnd's patch up then ? We should be fixing this in > > one of the upcoming -rcs since we introduced it in the last merge > > window. > > IIUC, a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort > hook") appeared in v5.15-rc1 and added a use of __clk_is_enabled(), > which is only available when COMMON_CLK=y. > > PCIE_RCAR_HOST depends on ARCH_RENESAS || COMPILE_TEST. ARCH_RENESAS > is an ARM64 platform, so when COMPILE_TEST is not set, I think we get > COMMON_CLK=y via this: > > config ARM64 > select COMMON_CLK > > But when ARCH_RENESAS is not set and COMPILE_TEST=y, there's nothing > that enforces the dependency on COMMON_CLK. Personally I like the > first hunk of Marek's patch at [1] because the dependency on > COMMON_CLK is explicit: > > config PCIE_RCAR_HOST > depends on ARCH_RENESAS || COMPILE_TEST > depends on COMMON_CLK Agreed, Marek's version is clearer than mine, please use that. Arnd
On Wed, Sep 29, 2021 at 8:55 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Sep 29, 2021 at 6:32 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Wed, Sep 29, 2021 at 03:55:09PM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Sep 22, 2021 at 01:13:11AM +0200, Marek Vasut wrote: > > > > On 9/21/21 6:08 PM, Geert Uytterhoeven wrote: > > > > > > > > [...] > > > > > > > > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > > > > > > index 326f7d13024f..ee6f5e525d3a 100644 > > > > > > --- a/drivers/pci/controller/Kconfig > > > > > > +++ b/drivers/pci/controller/Kconfig > > > > > > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > > > > > > config PCIE_RCAR_HOST > > > > > > bool "Renesas R-Car PCIe host controller" > > > > > > depends on ARCH_RENESAS || COMPILE_TEST > > > > > > + depends on COMMON_CLK > > > > > > > > > > > Bjorn, shall we pick Arnd's patch up then ? We should be fixing this in > > > one of the upcoming -rcs since we introduced it in the last merge > > > window. > > > > IIUC, a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort > > hook") appeared in v5.15-rc1 and added a use of __clk_is_enabled(), > > which is only available when COMMON_CLK=y. > > > > PCIE_RCAR_HOST depends on ARCH_RENESAS || COMPILE_TEST. ARCH_RENESAS > > is an ARM64 platform, so when COMPILE_TEST is not set, I think we get > > COMMON_CLK=y via this: > > > > config ARM64 > > select COMMON_CLK > > > > But when ARCH_RENESAS is not set and COMPILE_TEST=y, there's nothing > > that enforces the dependency on COMMON_CLK. Personally I like the > > first hunk of Marek's patch at [1] because the dependency on > > COMMON_CLK is explicit: > > > > config PCIE_RCAR_HOST > > depends on ARCH_RENESAS || COMPILE_TEST > > depends on COMMON_CLK > > Agreed, Marek's version is clearer than mine, please use that. But PCIE_RCAR_EP does not need the dependency. Gr{oetje,eeting}s, Geert
On Wed, Sep 29, 2021 at 09:08:42PM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 29, 2021 at 8:55 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Sep 29, 2021 at 6:32 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Wed, Sep 29, 2021 at 03:55:09PM +0100, Lorenzo Pieralisi wrote: > > > > On Wed, Sep 22, 2021 at 01:13:11AM +0200, Marek Vasut wrote: > > > > > On 9/21/21 6:08 PM, Geert Uytterhoeven wrote: > > > > > > > > > > [...] > > > > > > > > > > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > > > > > > > index 326f7d13024f..ee6f5e525d3a 100644 > > > > > > > --- a/drivers/pci/controller/Kconfig > > > > > > > +++ b/drivers/pci/controller/Kconfig > > > > > > > @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 > > > > > > > config PCIE_RCAR_HOST > > > > > > > bool "Renesas R-Car PCIe host controller" > > > > > > > depends on ARCH_RENESAS || COMPILE_TEST > > > > > > > + depends on COMMON_CLK > > > > > > > > > > > > > > Bjorn, shall we pick Arnd's patch up then ? We should be fixing this in > > > > one of the upcoming -rcs since we introduced it in the last merge > > > > window. > > > > > > IIUC, a115b1bd3af0 ("PCI: rcar: Add L1 link state fix into data abort > > > hook") appeared in v5.15-rc1 and added a use of __clk_is_enabled(), > > > which is only available when COMMON_CLK=y. > > > > > > PCIE_RCAR_HOST depends on ARCH_RENESAS || COMPILE_TEST. ARCH_RENESAS > > > is an ARM64 platform, so when COMPILE_TEST is not set, I think we get > > > COMMON_CLK=y via this: > > > > > > config ARM64 > > > select COMMON_CLK > > > > > > But when ARCH_RENESAS is not set and COMPILE_TEST=y, there's nothing > > > that enforces the dependency on COMMON_CLK. Personally I like the > > > first hunk of Marek's patch at [1] because the dependency on > > > COMMON_CLK is explicit: > > > > > > config PCIE_RCAR_HOST > > > depends on ARCH_RENESAS || COMPILE_TEST > > > depends on COMMON_CLK > > > > Agreed, Marek's version is clearer than mine, please use that. > > But PCIE_RCAR_EP does not need the dependency. Right, that's why I said the *first* hunk of Marek's patch. I would apply that and skip the second one.
+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. 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?
Hi Stephen, 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! ;-) > 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
Quoting Geert Uytterhoeven (2021-09-30 01:01:24) > Hi Stephen, > > 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?
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
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index 326f7d13024f..ee6f5e525d3a 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -66,6 +66,7 @@ config PCI_RCAR_GEN2 config PCIE_RCAR_HOST bool "Renesas R-Car PCIe host controller" depends on ARCH_RENESAS || COMPILE_TEST + depends on COMMON_CLK depends on PCI_MSI_IRQ_DOMAIN help Say Y here if you want PCIe controller support on R-Car SoCs in host @@ -74,6 +75,7 @@ config PCIE_RCAR_HOST config PCIE_RCAR_EP bool "Renesas R-Car PCIe endpoint controller" depends on ARCH_RENESAS || COMPILE_TEST + depends on COMMON_CLK depends on PCI_ENDPOINT help Say Y here if you want PCIe controller support on R-Car SoCs in