* [PATCH] PCI: rcar: Add missing COMMON_CLK dependency
@ 2021-09-07 14:45 marek.vasut
2021-09-13 12:39 ` Lorenzo Pieralisi
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: marek.vasut @ 2021-09-07 14:45 UTC (permalink / raw)
To: linux-pci
Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven,
Lorenzo Pieralisi, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda,
linux-renesas-soc
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
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-07 14:45 [PATCH] PCI: rcar: Add missing COMMON_CLK dependency marek.vasut @ 2021-09-13 12:39 ` Lorenzo Pieralisi 2021-09-21 16:08 ` Geert Uytterhoeven 2021-09-30 5:30 ` Stephen Boyd 2 siblings, 0 replies; 13+ messages in thread From: Lorenzo Pieralisi @ 2021-09-13 12:39 UTC (permalink / raw) To: marek.vasut, bhelgaas, Stephen Boyd Cc: linux-pci, Marek Vasut, Geert Uytterhoeven, Wolfram Sang, Yoshihiro Shimoda, linux-renesas-soc 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-07 14:45 [PATCH] PCI: rcar: Add missing COMMON_CLK dependency marek.vasut 2021-09-13 12:39 ` Lorenzo Pieralisi @ 2021-09-21 16:08 ` Geert Uytterhoeven 2021-09-21 23:13 ` Marek Vasut 2021-09-30 5:30 ` Stephen Boyd 2 siblings, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-21 16:08 UTC (permalink / raw) To: Marek Vasut Cc: linux-pci, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas 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 -- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-21 16:08 ` Geert Uytterhoeven @ 2021-09-21 23:13 ` Marek Vasut 2021-09-29 14:55 ` Lorenzo Pieralisi 0 siblings, 1 reply; 13+ messages in thread From: Marek Vasut @ 2021-09-21 23:13 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas, Arnd Bergmann 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-21 23:13 ` Marek Vasut @ 2021-09-29 14:55 ` Lorenzo Pieralisi 2021-09-29 16:32 ` Bjorn Helgaas 0 siblings, 1 reply; 13+ messages in thread From: Lorenzo Pieralisi @ 2021-09-29 14:55 UTC (permalink / raw) To: Marek Vasut, Bjorn Helgaas Cc: Geert Uytterhoeven, linux-pci, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas, Arnd Bergmann 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-29 14:55 ` Lorenzo Pieralisi @ 2021-09-29 16:32 ` Bjorn Helgaas 2021-09-29 18:55 ` Arnd Bergmann 0 siblings, 1 reply; 13+ messages in thread From: Bjorn Helgaas @ 2021-09-29 16:32 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven, linux-pci, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas, Arnd Bergmann 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/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-29 16:32 ` Bjorn Helgaas @ 2021-09-29 18:55 ` Arnd Bergmann 2021-09-29 19:08 ` Geert Uytterhoeven 0 siblings, 1 reply; 13+ messages in thread From: Arnd Bergmann @ 2021-09-29 18:55 UTC (permalink / raw) To: Bjorn Helgaas Cc: Lorenzo Pieralisi, Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven, linux-pci, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas, Arnd Bergmann 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-29 18:55 ` Arnd Bergmann @ 2021-09-29 19:08 ` Geert Uytterhoeven 2021-09-29 19:11 ` Bjorn Helgaas 0 siblings, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-29 19:08 UTC (permalink / raw) To: Arnd Bergmann Cc: Bjorn Helgaas, Lorenzo Pieralisi, Marek Vasut, Bjorn Helgaas, linux-pci, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas 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 -- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-29 19:08 ` Geert Uytterhoeven @ 2021-09-29 19:11 ` Bjorn Helgaas 0 siblings, 0 replies; 13+ messages in thread From: Bjorn Helgaas @ 2021-09-29 19:11 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Arnd Bergmann, Lorenzo Pieralisi, Marek Vasut, Bjorn Helgaas, linux-pci, Stephen Boyd, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-07 14:45 [PATCH] PCI: rcar: Add missing COMMON_CLK dependency marek.vasut 2021-09-13 12:39 ` Lorenzo Pieralisi 2021-09-21 16:08 ` Geert Uytterhoeven @ 2021-09-30 5:30 ` Stephen Boyd 2021-09-30 8:01 ` Geert Uytterhoeven 2 siblings, 1 reply; 13+ messages in thread From: Stephen Boyd @ 2021-09-30 5:30 UTC (permalink / raw) To: linux-pci, marek.vasut, linux-clk Cc: Marek Vasut, Bjorn Helgaas, Geert Uytterhoeven, Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda, linux-renesas-soc +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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-30 5:30 ` Stephen Boyd @ 2021-09-30 8:01 ` Geert Uytterhoeven 2021-09-30 18:16 ` Stephen Boyd 0 siblings, 1 reply; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-30 8:01 UTC (permalink / raw) To: Stephen Boyd Cc: linux-pci, Marek Vasut, linux-clk, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas 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 -- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-30 8:01 ` Geert Uytterhoeven @ 2021-09-30 18:16 ` Stephen Boyd 2021-09-30 18:31 ` Geert Uytterhoeven 0 siblings, 1 reply; 13+ messages in thread From: Stephen Boyd @ 2021-09-30 18:16 UTC (permalink / raw) To: Geert Uytterhoeven Cc: linux-pci, Marek Vasut, linux-clk, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] PCI: rcar: Add missing COMMON_CLK dependency 2021-09-30 18:16 ` Stephen Boyd @ 2021-09-30 18:31 ` Geert Uytterhoeven 0 siblings, 0 replies; 13+ messages in thread From: Geert Uytterhoeven @ 2021-09-30 18:31 UTC (permalink / raw) To: Stephen Boyd Cc: linux-pci, Marek Vasut, linux-clk, Marek Vasut, Bjorn Helgaas, Lorenzo Pieralisi, Wolfram Sang, Yoshihiro Shimoda, Linux-Renesas 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-30 18:31 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-07 14:45 [PATCH] PCI: rcar: Add missing COMMON_CLK dependency marek.vasut 2021-09-13 12:39 ` Lorenzo Pieralisi 2021-09-21 16:08 ` Geert Uytterhoeven 2021-09-21 23:13 ` Marek Vasut 2021-09-29 14:55 ` Lorenzo Pieralisi 2021-09-29 16:32 ` Bjorn Helgaas 2021-09-29 18:55 ` Arnd Bergmann 2021-09-29 19:08 ` Geert Uytterhoeven 2021-09-29 19:11 ` Bjorn Helgaas 2021-09-30 5:30 ` Stephen Boyd 2021-09-30 8:01 ` Geert Uytterhoeven 2021-09-30 18:16 ` Stephen Boyd 2021-09-30 18:31 ` Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.