[PATCH 0/3] Legacy clock drivers: Normalize clk API

[PATCH 0/3] Legacy clock drivers: Normalize clk API

[Geert Uytterhoeven]

	Hi all,

When seeing commit bde4975310eb1982 ("net: stmmac: fix build failure due
to missing COMMON_CLK dependency"), I wondered why this dependency is
needed, as all implementations of the clock API should implement all
required functionality, or provide dummies.

It turns out there were still two implementations that lacked the
clk_set_rate() function: Coldfire and AR7.

This series contains three patches:
  - The first two patches add dummies for clk_set_rate(),
    clk_set_rate(), clk_set_parent(), and clk_get_parent() to the
    Coldfire and AR7, like Arnd has done for other legacy clock
    implementations a while ago.
  - The second patch removes the COMMON_CLK dependency from the stmmac
    network drivers again, as it is no longer needed.
    Obviously this patch has a hard dependency on the first two patches.

Thanks!

Geert Uytterhoeven (3):
  m68k: coldfire: Normalize clk API
  MIPS: AR7: Normalize clk API
  [RFC] Revert "net: stmmac: fix build failure due to missing COMMON_CLK
    dependency"

 arch/m68k/coldfire/clk.c                    | 29 +++++++++++++++++++++++++++++
 arch/mips/ar7/clock.c                       | 29 +++++++++++++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig | 10 +++++-----
 3 files changed, 63 insertions(+), 5 deletions(-)

-- 
2.7.4

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

[PATCH 1/3] m68k: coldfire: Normalize clk API

[Geert Uytterhoeven]

Coldfire still provides its own variant of the clk API rather than using
the generic COMMON_CLK API.  This generally works, but it causes some
link errors with drivers using the clk_round_rate(), clk_set_rate(),
clk_set_parent(), or clk_get_parent() functions when a platform lacks
those interfaces.

This adds empty stub implementations for each of them, and I don't even
try to do something useful here but instead just print a WARN() message
to make it obvious what is going on if they ever end up being called.

The drivers that call these won't be used on these platforms (otherwise
we'd get a link error today), so the added code is harmless bloat and
will warn about accidental use.

Based on commit bd7fefe1f06ca6cc ("ARM: w90x900: normalize clk API").

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 arch/m68k/coldfire/clk.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/m68k/coldfire/clk.c b/arch/m68k/coldfire/clk.c
index 849cd208e2ed99e6..7bc666e482ebe82f 100644
--- a/arch/m68k/coldfire/clk.c
+++ b/arch/m68k/coldfire/clk.c
@@ -129,4 +129,33 @@ unsigned long clk_get_rate(struct clk *clk)
 }
 EXPORT_SYMBOL(clk_get_rate);
 
+/* dummy functions, should not be called */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	WARN_ON(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	WARN_ON(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	WARN_ON(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	WARN_ON(clk);
+	return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
 /***************************************************************************/
-- 
2.7.4

Re: [PATCH 1/3] m68k: coldfire: Normalize clk API

[Greg Ungerer]

Hi Geert,

On 11/06/18 18:44, Geert Uytterhoeven wrote:
> Coldfire still provides its own variant of the clk API rather than using
> the generic COMMON_CLK API.  This generally works, but it causes some
> link errors with drivers using the clk_round_rate(), clk_set_rate(),
> clk_set_parent(), or clk_get_parent() functions when a platform lacks
> those interfaces.
> 
> This adds empty stub implementations for each of them, and I don't even
> try to do something useful here but instead just print a WARN() message
> to make it obvious what is going on if they ever end up being called.
> 
> The drivers that call these won't be used on these platforms (otherwise
> we'd get a link error today), so the added code is harmless bloat and
> will warn about accidental use.
> 
> Based on commit bd7fefe1f06ca6cc ("ARM: w90x900: normalize clk API").
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

I am fine with this for ColdFire, so

Acked-by: Greg Ungerer <gerg@linux-m68k.org>

Are you going to take this/these via your m68k git tree?

Regards
Greg


> ---
>   arch/m68k/coldfire/clk.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/arch/m68k/coldfire/clk.c b/arch/m68k/coldfire/clk.c
> index 849cd208e2ed99e6..7bc666e482ebe82f 100644
> --- a/arch/m68k/coldfire/clk.c
> +++ b/arch/m68k/coldfire/clk.c
> @@ -129,4 +129,33 @@ unsigned long clk_get_rate(struct clk *clk)
>   }
>   EXPORT_SYMBOL(clk_get_rate);
>   
> +/* dummy functions, should not be called */
> +long clk_round_rate(struct clk *clk, unsigned long rate)
> +{
> +	WARN_ON(clk);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_round_rate);
> +
> +int clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> +	WARN_ON(clk);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_set_rate);
> +
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	WARN_ON(clk);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	WARN_ON(clk);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(clk_get_parent);
> +
>   /***************************************************************************/
> 

Re: [PATCH 1/3] m68k: coldfire: Normalize clk API

[Geert Uytterhoeven]

Hi Greg,

On Tue, Jun 12, 2018 at 9:27 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 11/06/18 18:44, Geert Uytterhoeven wrote:
> > Coldfire still provides its own variant of the clk API rather than using
> > the generic COMMON_CLK API.  This generally works, but it causes some
> > link errors with drivers using the clk_round_rate(), clk_set_rate(),
> > clk_set_parent(), or clk_get_parent() functions when a platform lacks
> > those interfaces.
> >
> > This adds empty stub implementations for each of them, and I don't even
> > try to do something useful here but instead just print a WARN() message
> > to make it obvious what is going on if they ever end up being called.
> >
> > The drivers that call these won't be used on these platforms (otherwise
> > we'd get a link error today), so the added code is harmless bloat and
> > will warn about accidental use.
> >
> > Based on commit bd7fefe1f06ca6cc ("ARM: w90x900: normalize clk API").
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> I am fine with this for ColdFire, so
>
> Acked-by: Greg Ungerer <gerg@linux-m68k.org>

Thanks!

> Are you going to take this/these via your m68k git tree?

I''m fine delagating this to you.
Thanks!

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

Re: [PATCH 1/3] m68k: coldfire: Normalize clk API

[Greg Ungerer]

Hi Geert,

On 12/06/18 17:31, Geert Uytterhoeven wrote:
> On Tue, Jun 12, 2018 at 9:27 AM Greg Ungerer <gerg@linux-m68k.org> wrote:
>> On 11/06/18 18:44, Geert Uytterhoeven wrote:
>>> Coldfire still provides its own variant of the clk API rather than using
>>> the generic COMMON_CLK API.  This generally works, but it causes some
>>> link errors with drivers using the clk_round_rate(), clk_set_rate(),
>>> clk_set_parent(), or clk_get_parent() functions when a platform lacks
>>> those interfaces.
>>>
>>> This adds empty stub implementations for each of them, and I don't even
>>> try to do something useful here but instead just print a WARN() message
>>> to make it obvious what is going on if they ever end up being called.
>>>
>>> The drivers that call these won't be used on these platforms (otherwise
>>> we'd get a link error today), so the added code is harmless bloat and
>>> will warn about accidental use.
>>>
>>> Based on commit bd7fefe1f06ca6cc ("ARM: w90x900: normalize clk API").
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>
>> I am fine with this for ColdFire, so
>>
>> Acked-by: Greg Ungerer <gerg@linux-m68k.org>
> 
> Thanks!
> 
>> Are you going to take this/these via your m68k git tree?
> 
> I''m fine delagating this to you.

No problem. I'll add it to the m68knommu git tree (for-next branch)
when the merge window closes.

Thanks
Greg

[PATCH 2/3] MIPS: AR7: Normalize clk API

[Geert Uytterhoeven]

Coldfire still provides its own variant of the clk API rather than using
the generic COMMON_CLK API.  This generally works, but it causes some
link errors with drivers using the clk_round_rate(), clk_set_rate(),
clk_set_parent(), or clk_get_parent() functions when a platform lacks
those interfaces.

This adds empty stub implementations for each of them, and I don't even
try to do something useful here but instead just print a WARN() message
to make it obvious what is going on if they ever end up being called.

The drivers that call these won't be used on these platforms (otherwise
we'd get a link error today), so the added code is harmless bloat and
will warn about accidental use.

Based on commit bd7fefe1f06ca6cc ("ARM: w90x900: normalize clk API").

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 arch/mips/ar7/clock.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/arch/mips/ar7/clock.c b/arch/mips/ar7/clock.c
index 0137656107a9c5b5..6b64fd96dba8fb26 100644
--- a/arch/mips/ar7/clock.c
+++ b/arch/mips/ar7/clock.c
@@ -476,3 +476,32 @@ void __init ar7_init_clocks(void)
 	/* adjust vbus clock rate */
 	vbus_clk.rate = bus_clk.rate / 2;
 }
+
+/* dummy functions, should not be called */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	WARN_ON(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	WARN_ON(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	WARN_ON(clk);
+	return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	WARN_ON(clk);
+	return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
-- 
2.7.4

Re: [PATCH 2/3] MIPS: AR7: Normalize clk API

[Paul Burton]

Hi Geert,

On Mon, Jun 11, 2018 at 10:44:22AM +0200, Geert Uytterhoeven wrote:
> Coldfire still provides its own variant of the clk API rather than using
> the generic COMMON_CLK API.  This generally works, but it causes some
> link errors with drivers using the clk_round_rate(), clk_set_rate(),
> clk_set_parent(), or clk_get_parent() functions when a platform lacks
> those interfaces.
> 
> This adds empty stub implementations for each of them, and I don't even
> try to do something useful here but instead just print a WARN() message
> to make it obvious what is going on if they ever end up being called.
> 
> The drivers that call these won't be used on these platforms (otherwise
> we'd get a link error today), so the added code is harmless bloat and
> will warn about accidental use.
> 
> Based on commit bd7fefe1f06ca6cc ("ARM: w90x900: normalize clk API").
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  arch/mips/ar7/clock.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Applied to mips-next for 4.19.

Thanks,
    Paul

[PATCH 3/3 RFC] Revert "net: stmmac: fix build failure due to missing COMMON_CLK dependency"

[Geert Uytterhoeven]

This reverts commit bde4975310eb1982bd0bbff673989052d92fd481.

All legacy clock implementations now implement clk_set_rate() (Some
implementations may be dummies, though).

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Marked "RFC", as this depends on "m68k: coldfire: Normalize clk API" and
"MIPS: AR7: Normalize clk API".
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index cb5b0f58c395c2bd..e28c0d2c58e911ed 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -33,7 +33,7 @@ config DWMAC_DWC_QOS_ETH
 	select PHYLIB
 	select CRC32
 	select MII
-	depends on OF && COMMON_CLK && HAS_DMA
+	depends on OF && HAS_DMA
 	help
 	  Support for chips using the snps,dwc-qos-ethernet.txt DT binding.
 
@@ -57,7 +57,7 @@ config DWMAC_ANARION
 config DWMAC_IPQ806X
 	tristate "QCA IPQ806x DWMAC support"
 	default ARCH_QCOM
-	depends on OF && COMMON_CLK && (ARCH_QCOM || COMPILE_TEST)
+	depends on OF && (ARCH_QCOM || COMPILE_TEST)
 	select MFD_SYSCON
 	help
 	  Support for QCA IPQ806X DWMAC Ethernet.
@@ -100,7 +100,7 @@ config DWMAC_OXNAS
 config DWMAC_ROCKCHIP
 	tristate "Rockchip dwmac support"
 	default ARCH_ROCKCHIP
-	depends on OF && COMMON_CLK && (ARCH_ROCKCHIP || COMPILE_TEST)
+	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
 	select MFD_SYSCON
 	help
 	  Support for Ethernet controller on Rockchip RK3288 SoC.
@@ -123,7 +123,7 @@ config DWMAC_SOCFPGA
 config DWMAC_STI
 	tristate "STi GMAC support"
 	default ARCH_STI
-	depends on OF && COMMON_CLK && (ARCH_STI || COMPILE_TEST)
+	depends on OF && (ARCH_STI || COMPILE_TEST)
 	select MFD_SYSCON
 	---help---
 	  Support for ethernet controller on STi SOCs.
@@ -147,7 +147,7 @@ config DWMAC_STM32
 config DWMAC_SUNXI
 	tristate "Allwinner GMAC support"
 	default ARCH_SUNXI
-	depends on OF && COMMON_CLK && (ARCH_SUNXI || COMPILE_TEST)
+	depends on OF && (ARCH_SUNXI || COMPILE_TEST)
 	---help---
 	  Support for Allwinner A20/A31 GMAC ethernet controllers.
 
-- 
2.7.4

Re: [PATCH 3/3 RFC] Revert "net: stmmac: fix build failure due to missing COMMON_CLK dependency"

[Arnd Bergmann]

On Mon, Jun 11, 2018 at 10:44 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> This reverts commit bde4975310eb1982bd0bbff673989052d92fd481.
>
> All legacy clock implementations now implement clk_set_rate() (Some
> implementations may be dummies, though).
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Marked "RFC", as this depends on "m68k: coldfire: Normalize clk API" and
> "MIPS: AR7: Normalize clk API".

This seems reasonable. It's possible that it will cause regressions because the
COMMON_CLK dependency hides another dependency on something else
that not everything implements, but we should fix that properly if that happens.

       Arnd

Re: [PATCH 3/3 RFC] Revert "net: stmmac: fix build failure due to missing COMMON_CLK dependency"

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Jun 11, 2018 at 10:59 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jun 11, 2018 at 10:44 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > This reverts commit bde4975310eb1982bd0bbff673989052d92fd481.
> >
> > All legacy clock implementations now implement clk_set_rate() (Some
> > implementations may be dummies, though).
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Marked "RFC", as this depends on "m68k: coldfire: Normalize clk API" and
> > "MIPS: AR7: Normalize clk API".
>
> This seems reasonable. It's possible that it will cause regressions because the
> COMMON_CLK dependency hides another dependency on something else
> that not everything implements, but we should fix that properly if that happens.

Compile-testing was enabled 2 years ago, in commit 2e280c188f06b190
("stmmac: make platform drivers depend on their associated SoC"), but the
dependency on COMMON_CLK was added only recently.
That's what triggered me: the drivers were suddenly disabled in m68k
allmodconfig,
while they built fine for years before.

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

Re: [PATCH 3/3 RFC] Revert "net: stmmac: fix build failure due to missing COMMON_CLK dependency"

[Jose Abreu]

Hi,

On 11-06-2018 09:44, Geert Uytterhoeven wrote:
> This reverts commit bde4975310eb1982bd0bbff673989052d92fd481.
>
> All legacy clock implementations now implement clk_set_rate() (Some
> implementations may be dummies, though).
>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>

This seems okay by me. You can send a non-rfc patch with my ack
once other 2 patches get accepted:

Acked-by: Jose Abreu <joabreu@synopsys.com>

Thanks and Best Regards,
Jose Miguel Abreu

Re: [PATCH 0/3] Legacy clock drivers: Normalize clk API

[Arnd Bergmann]

On Mon, Jun 11, 2018 at 10:44 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>         Hi all,
>
> When seeing commit bde4975310eb1982 ("net: stmmac: fix build failure due
> to missing COMMON_CLK dependency"), I wondered why this dependency is
> needed, as all implementations of the clock API should implement all
> required functionality, or provide dummies.
>
> It turns out there were still two implementations that lacked the
> clk_set_rate() function: Coldfire and AR7.
>
> This series contains three patches:
>   - The first two patches add dummies for clk_set_rate(),
>     clk_set_rate(), clk_set_parent(), and clk_get_parent() to the
>     Coldfire and AR7, like Arnd has done for other legacy clock
>     implementations a while ago.
>   - The second patch removes the COMMON_CLK dependency from the stmmac
>     network drivers again, as it is no longer needed.
>     Obviously this patch has a hard dependency on the first two patches.

Yes, good idea.

Acked-by: Arnd Bergmann <arnd@arnd.de>

One question: what happens on machines that don't support any CLK
interface, i.e.
that don't have any of COMMON_CLK/HAVE_CLK/CLKDEV_LOOKUP?

I guess those are already hopelessly broken for many drivers, right?

         Arnd

Re: [PATCH 0/3] Legacy clock drivers: Normalize clk API

[Geert Uytterhoeven]

Hi Arnd,

On Mon, Jun 11, 2018 at 11:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jun 11, 2018 at 10:44 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > When seeing commit bde4975310eb1982 ("net: stmmac: fix build failure due
> > to missing COMMON_CLK dependency"), I wondered why this dependency is
> > needed, as all implementations of the clock API should implement all
> > required functionality, or provide dummies.
> >
> > It turns out there were still two implementations that lacked the
> > clk_set_rate() function: Coldfire and AR7.
> >
> > This series contains three patches:
> >   - The first two patches add dummies for clk_set_rate(),
> >     clk_set_rate(), clk_set_parent(), and clk_get_parent() to the
> >     Coldfire and AR7, like Arnd has done for other legacy clock
> >     implementations a while ago.
> >   - The second patch removes the COMMON_CLK dependency from the stmmac
> >     network drivers again, as it is no longer needed.
> >     Obviously this patch has a hard dependency on the first two patches.
>
> Yes, good idea.
>
> Acked-by: Arnd Bergmann <arnd@arnd.de>

Thanks!

> One question: what happens on machines that don't support any CLK
> interface, i.e.
> that don't have any of COMMON_CLK/HAVE_CLK/CLKDEV_LOOKUP?
>
> I guess those are already hopelessly broken for many drivers, right?

Nope, they (e.g. m68k allmodconfig) build fine, as they don't define
CONFIG_HAVE_CLK.

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