PCI: rcar: Add missing COMMON_CLK dependency

Commit Message

Marek Vasut Sept. 7, 2021, 2:45 p.m. UTC

From: Marek Vasut <[e-mail]>

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 <[e-mail]>
Cc: Bjorn Helgaas <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Wolfram Sang <[e-mail]>
Cc: Yoshihiro Shimoda <[e-mail]>
Cc: [e-mail]
---
+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(+)

Comments

Lorenzo Pieralisi Sept. 13, 2021, 12:39 p.m. UTC | #1

On Tue, Sep 07, 2021 at 04:45:12PM +0200, [e-mail] wrote:
> From: Marek Vasut <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Wolfram Sang <[e-mail]>
> Cc: Yoshihiro Shimoda <[e-mail]>
> Cc: [e-mail]
> ---
> +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
>

Geert Uytterhoeven Sept. 21, 2021, 4:08 p.m. UTC | #2

Hi Marek,

Thanks for your patch!

On Tue, Sep 7, 2021 at 4:45 PM <[e-mail]> wrote:
> From: Marek Vasut <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Wolfram Sang <[e-mail]>
> Cc: Yoshihiro Shimoda <[e-mail]>
> Cc: [e-mail]
> ---
> +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

Marek Vasut Sept. 21, 2021, 11:13 p.m. UTC | #3

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/[e-mail]/

Lorenzo Pieralisi Sept. 29, 2021, 2:55 p.m. UTC | #4

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/[e-mail]/

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

Bjorn Helgaas Sept. 29, 2021, 4:32 p.m. UTC | #5

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/[e-mail]/
> 
> 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/[e-mail]/
[2] https://lore.kernel.org/all/[e-mail]/

Arnd Bergmann Sept. 29, 2021, 6:55 p.m. UTC | #6

On Wed, Sep 29, 2021 at 6:32 PM Bjorn Helgaas <[e-mail]> 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

Geert Uytterhoeven Sept. 29, 2021, 7:08 p.m. UTC | #7

On Wed, Sep 29, 2021 at 8:55 PM Arnd Bergmann <[e-mail]> wrote:
> On Wed, Sep 29, 2021 at 6:32 PM Bjorn Helgaas <[e-mail]> 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

Bjorn Helgaas Sept. 29, 2021, 7:11 p.m. UTC | #8

On Wed, Sep 29, 2021 at 09:08:42PM +0200, Geert Uytterhoeven wrote:
> On Wed, Sep 29, 2021 at 8:55 PM Arnd Bergmann <[e-mail]> wrote:
> > On Wed, Sep 29, 2021 at 6:32 PM Bjorn Helgaas <[e-mail]> 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.

Stephen Boyd Sept. 30, 2021, 5:30 a.m. UTC | #9

+linux-clk as I don't regularly read my inbox :/

Quoting [e-mail] (2021-09-07 07:45:12)
> From: Marek Vasut <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Bjorn Helgaas <[e-mail]>
> Cc: Geert Uytterhoeven <[e-mail]>
> Cc: Lorenzo Pieralisi <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Wolfram Sang <[e-mail]>
> Cc: Yoshihiro Shimoda <[e-mail]>
> Cc: [e-mail]
> ---
> +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?

Geert Uytterhoeven Sept. 30, 2021, 8:01 a.m. UTC | #10

Hi Stephen,

On Thu, Sep 30, 2021 at 7:30 AM Stephen Boyd <[e-mail]> wrote:
> +linux-clk as I don't regularly read my inbox :/
>
> Quoting [e-mail] (2021-09-07 07:45:12)
> > From: Marek Vasut <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Bjorn Helgaas <[e-mail]>
> > Cc: Geert Uytterhoeven <[e-mail]>
> > Cc: Lorenzo Pieralisi <[e-mail]>
> > Cc: Stephen Boyd <[e-mail]>
> > Cc: Wolfram Sang <[e-mail]>
> > Cc: Yoshihiro Shimoda <[e-mail]>
> > Cc: [e-mail]
> > ---
> > +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

Stephen Boyd Sept. 30, 2021, 6:16 p.m. UTC | #11

Quoting Geert Uytterhoeven (2021-09-30 01:01:24)
> Hi Stephen,
> 
> On Thu, Sep 30, 2021 at 7:30 AM Stephen Boyd <[e-mail]> wrote:
> > +linux-clk as I don't regularly read my inbox :/
> >
> > Quoting [e-mail] (2021-09-07 07:45:12)
> > > From: Marek Vasut <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Bjorn Helgaas <[e-mail]>
> > > Cc: Geert Uytterhoeven <[e-mail]>
> > > Cc: Lorenzo Pieralisi <[e-mail]>
> > > Cc: Stephen Boyd <[e-mail]>
> > > Cc: Wolfram Sang <[e-mail]>
> > > Cc: Yoshihiro Shimoda <[e-mail]>
> > > Cc: [e-mail]
> > > ---
> > > +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?

Geert Uytterhoeven Sept. 30, 2021, 6:31 p.m. UTC | #12

Hi Stephen,

On Thu, Sep 30, 2021 at 8:16 PM Stephen Boyd <[e-mail]> wrote:
> Quoting Geert Uytterhoeven (2021-09-30 01:01:24)
> > On Thu, Sep 30, 2021 at 7:30 AM Stephen Boyd <[e-mail]> wrote:
> > > +linux-clk as I don't regularly read my inbox :/
> > >
> > > Quoting [e-mail] (2021-09-07 07:45:12)
> > > > From: Marek Vasut <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Cc: Bjorn Helgaas <[e-mail]>
> > > > Cc: Geert Uytterhoeven <[e-mail]>
> > > > Cc: Lorenzo Pieralisi <[e-mail]>
> > > > Cc: Stephen Boyd <[e-mail]>
> > > > Cc: Wolfram Sang <[e-mail]>
> > > > Cc: Yoshihiro Shimoda <[e-mail]>
> > > > Cc: [e-mail]
> > > > ---
> > > > +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

Patch

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