* [PATCH] ARM: sa1100: convert to common clock framework
@ 2019-06-29 10:14 Russell King
2019-07-18 16:38 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Russell King @ 2019-06-29 10:14 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd; +Cc: linux-clk, linux-arm-kernel
Convert sa1100 to use the common clock framework.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
Please ack; this is part of a larger series. Thanks.
arch/arm/Kconfig | 1 +
arch/arm/mach-sa1100/clock.c | 221 +++++++++++++++++++------------------------
2 files changed, 96 insertions(+), 126 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9aed25a6019b..005549cbc963 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -549,6 +549,7 @@ config ARCH_SA1100
select CLKSRC_MMIO
select CLKSRC_PXA
select TIMER_OF if OF
+ select COMMON_CLK
select CPU_FREQ
select CPU_SA1100
select GENERIC_CLOCKEVENTS
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index 6199e87447ca..523ef25618f7 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -2,176 +2,145 @@
/*
* linux/arch/arm/mach-sa1100/clock.c
*/
-#include <linux/module.h>
#include <linux/kernel.h>
-#include <linux/device.h>
-#include <linux/list.h>
#include <linux/errno.h>
#include <linux/err.h>
-#include <linux/string.h>
#include <linux/clk.h>
-#include <linux/spinlock.h>
-#include <linux/mutex.h>
-#include <linux/io.h>
#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
#include <mach/hardware.h>
#include <mach/generic.h>
-struct clkops {
- void (*enable)(struct clk *);
- void (*disable)(struct clk *);
- unsigned long (*get_rate)(struct clk *);
+static const char * const clk_tucr_parents[] = {
+ "clk32768", "clk3686400",
};
-struct clk {
- const struct clkops *ops;
- unsigned int enabled;
-};
-
-#define DEFINE_CLK(_name, _ops) \
-struct clk clk_##_name = { \
- .ops = _ops, \
- }
-
-static DEFINE_SPINLOCK(clocks_lock);
-
-/* Dummy clk routine to build generic kernel parts that may be using them */
-long clk_round_rate(struct clk *clk, unsigned long rate)
-{
- return clk_get_rate(clk);
-}
-EXPORT_SYMBOL(clk_round_rate);
-
-int clk_set_rate(struct clk *clk, unsigned long rate)
-{
- return 0;
-}
-EXPORT_SYMBOL(clk_set_rate);
-
-int clk_set_parent(struct clk *clk, struct clk *parent)
-{
- return 0;
-}
-EXPORT_SYMBOL(clk_set_parent);
+static DEFINE_SPINLOCK(tucr_lock);
-struct clk *clk_get_parent(struct clk *clk)
+static int clk_gpio27_enable(struct clk_hw *hw)
{
- return NULL;
-}
-EXPORT_SYMBOL(clk_get_parent);
+ unsigned long flags;
-static void clk_gpio27_enable(struct clk *clk)
-{
/*
* First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
* (SA-1110 Developer's Manual, section 9.1.2.1)
*/
+ local_irq_save(flags);
GAFR |= GPIO_32_768kHz;
GPDR |= GPIO_32_768kHz;
- TUCR = TUCR_3_6864MHz;
+ local_irq_restore(flags);
+
+ return 0;
}
-static void clk_gpio27_disable(struct clk *clk)
+static void clk_gpio27_disable(struct clk_hw *hw)
{
- TUCR = 0;
+ unsigned long flags;
+
+ local_irq_save(flags);
GPDR &= ~GPIO_32_768kHz;
GAFR &= ~GPIO_32_768kHz;
+ local_irq_restore(flags);
}
-static void clk_cpu_enable(struct clk *clk)
-{
-}
+static const struct clk_ops clk_gpio27_ops = {
+ .enable = clk_gpio27_enable,
+ .disable = clk_gpio27_disable,
+};
-static void clk_cpu_disable(struct clk *clk)
-{
-}
+static const char * const clk_gpio27_parents[] = {
+ "tucr-mux",
+};
-static unsigned long clk_cpu_get_rate(struct clk *clk)
+static const struct clk_init_data clk_gpio27_init_data __initconst = {
+ .name = "gpio27",
+ .ops = &clk_gpio27_ops,
+ .parent_names = clk_gpio27_parents,
+ .num_parents = ARRAY_SIZE(clk_gpio27_parents),
+ .flags = CLK_IS_BASIC,
+};
+
+/*
+ * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
+ * multiply its input rate by 4 x (4 + PPCR). This calculation gives
+ * the exact rate. The figures given in the table are the rates rounded
+ * to 100kHz. Stick with sa11x0_getspeed() for the time being.
+ */
+static unsigned long clk_mpll_recalc_rate(struct clk_hw *hw,
+ unsigned long prate)
{
return sa11x0_getspeed(0) * 1000;
}
-int clk_enable(struct clk *clk)
-{
- unsigned long flags;
-
- if (clk) {
- spin_lock_irqsave(&clocks_lock, flags);
- if (clk->enabled++ == 0)
- clk->ops->enable(clk);
- spin_unlock_irqrestore(&clocks_lock, flags);
- }
-
- return 0;
-}
-EXPORT_SYMBOL(clk_enable);
+static const struct clk_ops clk_mpll_ops = {
+ .recalc_rate = clk_mpll_recalc_rate,
+};
-void clk_disable(struct clk *clk)
-{
- unsigned long flags;
+static const char * const clk_mpll_parents[] = {
+ "clk3686400",
+};
- if (clk) {
- WARN_ON(clk->enabled == 0);
- spin_lock_irqsave(&clocks_lock, flags);
- if (--clk->enabled == 0)
- clk->ops->disable(clk);
- spin_unlock_irqrestore(&clocks_lock, flags);
- }
-}
-EXPORT_SYMBOL(clk_disable);
+static const struct clk_init_data clk_mpll_init_data __initconst = {
+ .name = "mpll",
+ .ops = &clk_mpll_ops,
+ .parent_names = clk_mpll_parents,
+ .num_parents = ARRAY_SIZE(clk_mpll_parents),
+ .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,
+};
-unsigned long clk_get_rate(struct clk *clk)
+int __init sa11xx_clk_init(void)
{
- if (clk && clk->ops && clk->ops->get_rate)
- return clk->ops->get_rate(clk);
-
- return 0;
-}
-EXPORT_SYMBOL(clk_get_rate);
+ struct clk_hw *hw;
+ int ret;
-const struct clkops clk_gpio27_ops = {
- .enable = clk_gpio27_enable,
- .disable = clk_gpio27_disable,
-};
+ hw = clk_hw_register_fixed_rate(NULL, "clk32768", NULL, 0, 32768);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
-const struct clkops clk_cpu_ops = {
- .enable = clk_cpu_enable,
- .disable = clk_cpu_disable,
- .get_rate = clk_cpu_get_rate,
-};
+ clk_hw_register_clkdev(hw, NULL, "sa1100-rtc");
-static DEFINE_CLK(gpio27, &clk_gpio27_ops);
+ hw = clk_hw_register_fixed_rate(NULL, "clk3686400", NULL, 0, 3686400);
+ if (IS_ERR(hw))
+ return PTR_ERR(hw);
-static DEFINE_CLK(cpu, &clk_cpu_ops);
+ clk_hw_register_clkdev(hw, "OSTIMER0", NULL);
-static unsigned long clk_36864_get_rate(struct clk *clk)
-{
- return 3686400;
-}
+ hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+ if (!hw)
+ return -ENOMEM;
+ hw->init = &clk_mpll_init_data;
+ ret = clk_hw_register(NULL, hw);
+ if (ret) {
+ kfree(hw);
+ return ret;
+ }
-static struct clkops clk_36864_ops = {
- .enable = clk_cpu_enable,
- .disable = clk_cpu_disable,
- .get_rate = clk_36864_get_rate,
-};
+ clk_hw_register_clkdev(hw, NULL, "sa11x0-fb");
+ clk_hw_register_clkdev(hw, NULL, "sa11x0-pcmcia");
+ clk_hw_register_clkdev(hw, NULL, "sa11x0-pcmcia.0");
+ clk_hw_register_clkdev(hw, NULL, "sa11x0-pcmcia.1");
+ clk_hw_register_clkdev(hw, NULL, "1800");
+
+ hw = clk_hw_register_mux(NULL, "tucr-mux", clk_tucr_parents,
+ ARRAY_SIZE(clk_tucr_parents), 0,
+ (void __iomem *)&TUCR, FShft(TUCR_TSEL),
+ FAlnMsk(TUCR_TSEL), 0, &tucr_lock);
+ clk_set_rate(hw->clk, 3686400);
+
+ hw = kzalloc(sizeof(*hw), GFP_KERNEL);
+ if (!hw)
+ return -ENOMEM;
+ hw->init = &clk_gpio27_init_data;
+ ret = clk_hw_register(NULL, hw);
+ if (ret) {
+ kfree(hw);
+ return ret;
+ }
-static DEFINE_CLK(36864, &clk_36864_ops);
-
-static struct clk_lookup sa11xx_clkregs[] = {
- CLKDEV_INIT("sa1111.0", NULL, &clk_gpio27),
- CLKDEV_INIT("sa1100-rtc", NULL, NULL),
- CLKDEV_INIT("sa11x0-fb", NULL, &clk_cpu),
- CLKDEV_INIT("sa11x0-pcmcia", NULL, &clk_cpu),
- CLKDEV_INIT("sa11x0-pcmcia.0", NULL, &clk_cpu),
- CLKDEV_INIT("sa11x0-pcmcia.1", NULL, &clk_cpu),
- /* sa1111 names devices using internal offsets, PCMCIA is at 0x1800 */
- CLKDEV_INIT("1800", NULL, &clk_cpu),
- CLKDEV_INIT(NULL, "OSTIMER0", &clk_36864),
-};
+ clk_hw_register_clkdev(hw, NULL, "sa1111.0");
-int __init sa11xx_clk_init(void)
-{
- clkdev_add_table(sa11xx_clkregs, ARRAY_SIZE(sa11xx_clkregs));
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ARM: sa1100: convert to common clock framework
2019-06-29 10:14 [PATCH] ARM: sa1100: convert to common clock framework Russell King
@ 2019-07-18 16:38 ` Stephen Boyd
2019-07-18 17:49 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2019-07-18 16:38 UTC (permalink / raw)
To: Michael Turquette, Russell King; +Cc: linux-clk, linux-arm-kernel
Quoting Russell King (2019-06-29 03:14:10)
> Convert sa1100 to use the common clock framework.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> Please ack; this is part of a larger series. Thanks.
Just a few minor comments but otherwise looks good to me.
> diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
> index 6199e87447ca..523ef25618f7 100644
> --- a/arch/arm/mach-sa1100/clock.c
> +++ b/arch/arm/mach-sa1100/clock.c
> +static const char * const clk_tucr_parents[] = {
> + "clk32768", "clk3686400",
> };
It would be great if you used the new way of specifying clk parents with
direct pointers instead of strings. See commit fc0c209c147f ("clk: Allow
parents to be specified without string names") for some details.
>
> -struct clk {
> - const struct clkops *ops;
> - unsigned int enabled;
> -};
> -
> -#define DEFINE_CLK(_name, _ops) \
> -struct clk clk_##_name = { \
> - .ops = _ops, \
> - }
> -
> -static DEFINE_SPINLOCK(clocks_lock);
> -
> -/* Dummy clk routine to build generic kernel parts that may be using them */
> -long clk_round_rate(struct clk *clk, unsigned long rate)
> -{
> - return clk_get_rate(clk);
> -}
> -EXPORT_SYMBOL(clk_round_rate);
> -
> -int clk_set_rate(struct clk *clk, unsigned long rate)
> -{
> - return 0;
> -}
> -EXPORT_SYMBOL(clk_set_rate);
> -
> -int clk_set_parent(struct clk *clk, struct clk *parent)
> -{
> - return 0;
> -}
> -EXPORT_SYMBOL(clk_set_parent);
> +static DEFINE_SPINLOCK(tucr_lock);
>
> -struct clk *clk_get_parent(struct clk *clk)
> +static int clk_gpio27_enable(struct clk_hw *hw)
> {
> - return NULL;
> -}
> -EXPORT_SYMBOL(clk_get_parent);
> + unsigned long flags;
>
> -static void clk_gpio27_enable(struct clk *clk)
> -{
> /*
> * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
> * (SA-1110 Developer's Manual, section 9.1.2.1)
> */
> + local_irq_save(flags);
> GAFR |= GPIO_32_768kHz;
> GPDR |= GPIO_32_768kHz;
> - TUCR = TUCR_3_6864MHz;
> + local_irq_restore(flags);
> +
> + return 0;
> }
>
> -static void clk_gpio27_disable(struct clk *clk)
> +static void clk_gpio27_disable(struct clk_hw *hw)
> {
> - TUCR = 0;
> + unsigned long flags;
> +
> + local_irq_save(flags);
Why just disable irqs here?
> GPDR &= ~GPIO_32_768kHz;
> GAFR &= ~GPIO_32_768kHz;
> + local_irq_restore(flags);
> }
>
> -static void clk_cpu_enable(struct clk *clk)
> -{
> -}
> +static const struct clk_ops clk_gpio27_ops = {
> + .enable = clk_gpio27_enable,
> + .disable = clk_gpio27_disable,
> +};
>
> -static void clk_cpu_disable(struct clk *clk)
> -{
> -}
> +static const char * const clk_gpio27_parents[] = {
> + "tucr-mux",
> +};
>
> -static unsigned long clk_cpu_get_rate(struct clk *clk)
> +static const struct clk_init_data clk_gpio27_init_data __initconst = {
> + .name = "gpio27",
> + .ops = &clk_gpio27_ops,
> + .parent_names = clk_gpio27_parents,
> + .num_parents = ARRAY_SIZE(clk_gpio27_parents),
> + .flags = CLK_IS_BASIC,
CLK_IS_BASIC is gone. Please don't use it.
> +};
> +
> +/*
> + * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
> + * multiply its input rate by 4 x (4 + PPCR). This calculation gives
> + * the exact rate. The figures given in the table are the rates rounded
> + * to 100kHz. Stick with sa11x0_getspeed() for the time being.
[...]
> +static const struct clk_init_data clk_mpll_init_data __initconst = {
> + .name = "mpll",
> + .ops = &clk_mpll_ops,
> + .parent_names = clk_mpll_parents,
> + .num_parents = ARRAY_SIZE(clk_mpll_parents),
> + .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,
Please add a comment about these last two flags so we know why the rate
can't be cached and the clk is critical.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ARM: sa1100: convert to common clock framework
2019-07-18 16:38 ` Stephen Boyd
@ 2019-07-18 17:49 ` Russell King - ARM Linux admin
2019-07-18 18:43 ` Stephen Boyd
0 siblings, 1 reply; 5+ messages in thread
From: Russell King - ARM Linux admin @ 2019-07-18 17:49 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, linux-arm-kernel
On Thu, Jul 18, 2019 at 09:38:08AM -0700, Stephen Boyd wrote:
> Quoting Russell King (2019-06-29 03:14:10)
> > Convert sa1100 to use the common clock framework.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > Please ack; this is part of a larger series. Thanks.
>
> Just a few minor comments but otherwise looks good to me.
>
> > diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
> > index 6199e87447ca..523ef25618f7 100644
> > --- a/arch/arm/mach-sa1100/clock.c
> > +++ b/arch/arm/mach-sa1100/clock.c
> > +static const char * const clk_tucr_parents[] = {
> > + "clk32768", "clk3686400",
> > };
>
> It would be great if you used the new way of specifying clk parents with
> direct pointers instead of strings. See commit fc0c209c147f ("clk: Allow
> parents to be specified without string names") for some details.
I don't see at the moment how this is used with clk-mux.c - can you
provide some hints?
>
> >
> > -struct clk {
> > - const struct clkops *ops;
> > - unsigned int enabled;
> > -};
> > -
> > -#define DEFINE_CLK(_name, _ops) \
> > -struct clk clk_##_name = { \
> > - .ops = _ops, \
> > - }
> > -
> > -static DEFINE_SPINLOCK(clocks_lock);
> > -
> > -/* Dummy clk routine to build generic kernel parts that may be using them */
> > -long clk_round_rate(struct clk *clk, unsigned long rate)
> > -{
> > - return clk_get_rate(clk);
> > -}
> > -EXPORT_SYMBOL(clk_round_rate);
> > -
> > -int clk_set_rate(struct clk *clk, unsigned long rate)
> > -{
> > - return 0;
> > -}
> > -EXPORT_SYMBOL(clk_set_rate);
> > -
> > -int clk_set_parent(struct clk *clk, struct clk *parent)
> > -{
> > - return 0;
> > -}
> > -EXPORT_SYMBOL(clk_set_parent);
> > +static DEFINE_SPINLOCK(tucr_lock);
> >
> > -struct clk *clk_get_parent(struct clk *clk)
> > +static int clk_gpio27_enable(struct clk_hw *hw)
> > {
> > - return NULL;
> > -}
> > -EXPORT_SYMBOL(clk_get_parent);
> > + unsigned long flags;
> >
> > -static void clk_gpio27_enable(struct clk *clk)
> > -{
> > /*
> > * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
> > * (SA-1110 Developer's Manual, section 9.1.2.1)
> > */
> > + local_irq_save(flags);
> > GAFR |= GPIO_32_768kHz;
> > GPDR |= GPIO_32_768kHz;
> > - TUCR = TUCR_3_6864MHz;
> > + local_irq_restore(flags);
> > +
> > + return 0;
> > }
> >
> > -static void clk_gpio27_disable(struct clk *clk)
> > +static void clk_gpio27_disable(struct clk_hw *hw)
> > {
> > - TUCR = 0;
> > + unsigned long flags;
> > +
> > + local_irq_save(flags);
>
> Why just disable irqs here?
What do you mean? Do you mean "why are you only disabling IRQs and not
taking a spinlock" or do you mean "why are you disabling IRQs here" ?
>
> > GPDR &= ~GPIO_32_768kHz;
> > GAFR &= ~GPIO_32_768kHz;
> > + local_irq_restore(flags);
> > }
> >
> > -static void clk_cpu_enable(struct clk *clk)
> > -{
> > -}
> > +static const struct clk_ops clk_gpio27_ops = {
> > + .enable = clk_gpio27_enable,
> > + .disable = clk_gpio27_disable,
> > +};
> >
> > -static void clk_cpu_disable(struct clk *clk)
> > -{
> > -}
> > +static const char * const clk_gpio27_parents[] = {
> > + "tucr-mux",
> > +};
> >
> > -static unsigned long clk_cpu_get_rate(struct clk *clk)
> > +static const struct clk_init_data clk_gpio27_init_data __initconst = {
> > + .name = "gpio27",
> > + .ops = &clk_gpio27_ops,
> > + .parent_names = clk_gpio27_parents,
> > + .num_parents = ARRAY_SIZE(clk_gpio27_parents),
> > + .flags = CLK_IS_BASIC,
>
> CLK_IS_BASIC is gone. Please don't use it.
The patch is against 5.1, and you're right, so that was removed for the
version that ended up going upstream.
>
> > +};
> > +
> > +/*
> > + * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
> > + * multiply its input rate by 4 x (4 + PPCR). This calculation gives
> > + * the exact rate. The figures given in the table are the rates rounded
> > + * to 100kHz. Stick with sa11x0_getspeed() for the time being.
> [...]
> > +static const struct clk_init_data clk_mpll_init_data __initconst = {
> > + .name = "mpll",
> > + .ops = &clk_mpll_ops,
> > + .parent_names = clk_mpll_parents,
> > + .num_parents = ARRAY_SIZE(clk_mpll_parents),
> > + .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,
>
> Please add a comment about these last two flags so we know why the rate
> can't be cached and the clk is critical.
Ok, I'll do that with a follow-up patch once the merge window is over.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ARM: sa1100: convert to common clock framework
2019-07-18 17:49 ` Russell King - ARM Linux admin
@ 2019-07-18 18:43 ` Stephen Boyd
2019-07-18 22:41 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2019-07-18 18:43 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Michael Turquette, linux-clk, linux-arm-kernel
Quoting Russell King - ARM Linux admin (2019-07-18 10:49:01)
> On Thu, Jul 18, 2019 at 09:38:08AM -0700, Stephen Boyd wrote:
> > Quoting Russell King (2019-06-29 03:14:10)
> > > Convert sa1100 to use the common clock framework.
> > >
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > > Please ack; this is part of a larger series. Thanks.
> >
> > Just a few minor comments but otherwise looks good to me.
> >
> > > diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
> > > index 6199e87447ca..523ef25618f7 100644
> > > --- a/arch/arm/mach-sa1100/clock.c
> > > +++ b/arch/arm/mach-sa1100/clock.c
> > > +static const char * const clk_tucr_parents[] = {
> > > + "clk32768", "clk3686400",
> > > };
> >
> > It would be great if you used the new way of specifying clk parents with
> > direct pointers instead of strings. See commit fc0c209c147f ("clk: Allow
> > parents to be specified without string names") for some details.
>
> I don't see at the moment how this is used with clk-mux.c - can you
> provide some hints?
In this case both the parents are clk_hw pointers I think so an array
where first element is the clk_hw pointer to clk32768 and the second
element is the clk_hw pointer to clk3686400 would be assigned to
clk_init_data's parent_hws member.
struct clk_hw *clk_tucr_parents[] = {
&clk32768_hw,
&clk3686400_hw,
};
clk_tucr_init.parent_hws = clk_tucr_parents;
>
> > >
> > > -static void clk_gpio27_enable(struct clk *clk)
> > > -{
> > > /*
> > > * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
> > > * (SA-1110 Developer's Manual, section 9.1.2.1)
> > > */
> > > + local_irq_save(flags);
> > > GAFR |= GPIO_32_768kHz;
> > > GPDR |= GPIO_32_768kHz;
> > > - TUCR = TUCR_3_6864MHz;
> > > + local_irq_restore(flags);
> > > +
> > > + return 0;
> > > }
> > >
> > > -static void clk_gpio27_disable(struct clk *clk)
> > > +static void clk_gpio27_disable(struct clk_hw *hw)
> > > {
> > > - TUCR = 0;
> > > + unsigned long flags;
> > > +
> > > + local_irq_save(flags);
> >
> > Why just disable irqs here?
>
> What do you mean? Do you mean "why are you only disabling IRQs and not
> taking a spinlock" or do you mean "why are you disabling IRQs here" ?
I mean, why are you disabling irqs and not taking a spinlock? Must be
because there's already a spinlock in the clk framework?
>
> >
> > > GPDR &= ~GPIO_32_768kHz;
> > > GAFR &= ~GPIO_32_768kHz;
> > > + local_irq_restore(flags);
> > > }
> > >
> > > -static void clk_cpu_enable(struct clk *clk)
> > > -{
> > > -}
> > > +static const struct clk_ops clk_gpio27_ops = {
> > > + .enable = clk_gpio27_enable,
> > > + .disable = clk_gpio27_disable,
> > > +};
> > >
> > > -static void clk_cpu_disable(struct clk *clk)
> > > -{
> > > -}
> > > +static const char * const clk_gpio27_parents[] = {
> > > + "tucr-mux",
> > > +};
> > >
> > > -static unsigned long clk_cpu_get_rate(struct clk *clk)
> > > +static const struct clk_init_data clk_gpio27_init_data __initconst = {
> > > + .name = "gpio27",
> > > + .ops = &clk_gpio27_ops,
> > > + .parent_names = clk_gpio27_parents,
> > > + .num_parents = ARRAY_SIZE(clk_gpio27_parents),
> > > + .flags = CLK_IS_BASIC,
> >
> > CLK_IS_BASIC is gone. Please don't use it.
>
> The patch is against 5.1, and you're right, so that was removed for the
> version that ended up going upstream.
Oh did this get sent to Linus already? I guess I should have reviewed
this earlier.
>
> >
> > > +};
> > > +
> > > +/*
> > > + * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
> > > + * multiply its input rate by 4 x (4 + PPCR). This calculation gives
> > > + * the exact rate. The figures given in the table are the rates rounded
> > > + * to 100kHz. Stick with sa11x0_getspeed() for the time being.
> > [...]
> > > +static const struct clk_init_data clk_mpll_init_data __initconst = {
> > > + .name = "mpll",
> > > + .ops = &clk_mpll_ops,
> > > + .parent_names = clk_mpll_parents,
> > > + .num_parents = ARRAY_SIZE(clk_mpll_parents),
> > > + .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,
> >
> > Please add a comment about these last two flags so we know why the rate
> > can't be cached and the clk is critical.
>
> Ok, I'll do that with a follow-up patch once the merge window is over.
>
Ok, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ARM: sa1100: convert to common clock framework
2019-07-18 18:43 ` Stephen Boyd
@ 2019-07-18 22:41 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 5+ messages in thread
From: Russell King - ARM Linux admin @ 2019-07-18 22:41 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Michael Turquette, linux-clk, linux-arm-kernel
On Thu, Jul 18, 2019 at 11:43:07AM -0700, Stephen Boyd wrote:
> Quoting Russell King - ARM Linux admin (2019-07-18 10:49:01)
> > On Thu, Jul 18, 2019 at 09:38:08AM -0700, Stephen Boyd wrote:
> > > Quoting Russell King (2019-06-29 03:14:10)
> > > > Convert sa1100 to use the common clock framework.
> > > >
> > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > > Please ack; this is part of a larger series. Thanks.
> > >
> > > Just a few minor comments but otherwise looks good to me.
> > >
> > > > diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
> > > > index 6199e87447ca..523ef25618f7 100644
> > > > --- a/arch/arm/mach-sa1100/clock.c
> > > > +++ b/arch/arm/mach-sa1100/clock.c
> > > > +static const char * const clk_tucr_parents[] = {
> > > > + "clk32768", "clk3686400",
> > > > };
> > >
> > > It would be great if you used the new way of specifying clk parents with
> > > direct pointers instead of strings. See commit fc0c209c147f ("clk: Allow
> > > parents to be specified without string names") for some details.
> >
> > I don't see at the moment how this is used with clk-mux.c - can you
> > provide some hints?
>
> In this case both the parents are clk_hw pointers I think so an array
> where first element is the clk_hw pointer to clk32768 and the second
> element is the clk_hw pointer to clk3686400 would be assigned to
> clk_init_data's parent_hws member.
>
>
> struct clk_hw *clk_tucr_parents[] = {
> &clk32768_hw,
> &clk3686400_hw,
> };
>
> clk_tucr_init.parent_hws = clk_tucr_parents;
Thanks.
> > > > -static void clk_gpio27_enable(struct clk *clk)
> > > > -{
> > > > /*
> > > > * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
> > > > * (SA-1110 Developer's Manual, section 9.1.2.1)
> > > > */
> > > > + local_irq_save(flags);
> > > > GAFR |= GPIO_32_768kHz;
> > > > GPDR |= GPIO_32_768kHz;
> > > > - TUCR = TUCR_3_6864MHz;
> > > > + local_irq_restore(flags);
> > > > +
> > > > + return 0;
> > > > }
> > > >
> > > > -static void clk_gpio27_disable(struct clk *clk)
> > > > +static void clk_gpio27_disable(struct clk_hw *hw)
> > > > {
> > > > - TUCR = 0;
> > > > + unsigned long flags;
> > > > +
> > > > + local_irq_save(flags);
> > >
> > > Why just disable irqs here?
> >
> > What do you mean? Do you mean "why are you only disabling IRQs and not
> > taking a spinlock" or do you mean "why are you disabling IRQs here" ?
>
> I mean, why are you disabling irqs and not taking a spinlock? Must be
> because there's already a spinlock in the clk framework?
Nope - it's because there's no point taking a spinlock on something
that is fundamentally only a uniprocessor architecture. There's never
going to be a SA11x0 compatible SoC that has more than one core.
> > > > GPDR &= ~GPIO_32_768kHz;
> > > > GAFR &= ~GPIO_32_768kHz;
> > > > + local_irq_restore(flags);
> > > > }
> > > >
> > > > -static void clk_cpu_enable(struct clk *clk)
> > > > -{
> > > > -}
> > > > +static const struct clk_ops clk_gpio27_ops = {
> > > > + .enable = clk_gpio27_enable,
> > > > + .disable = clk_gpio27_disable,
> > > > +};
> > > >
> > > > -static void clk_cpu_disable(struct clk *clk)
> > > > -{
> > > > -}
> > > > +static const char * const clk_gpio27_parents[] = {
> > > > + "tucr-mux",
> > > > +};
> > > >
> > > > -static unsigned long clk_cpu_get_rate(struct clk *clk)
> > > > +static const struct clk_init_data clk_gpio27_init_data __initconst = {
> > > > + .name = "gpio27",
> > > > + .ops = &clk_gpio27_ops,
> > > > + .parent_names = clk_gpio27_parents,
> > > > + .num_parents = ARRAY_SIZE(clk_gpio27_parents),
> > > > + .flags = CLK_IS_BASIC,
> > >
> > > CLK_IS_BASIC is gone. Please don't use it.
> >
> > The patch is against 5.1, and you're right, so that was removed for the
> > version that ended up going upstream.
>
> Oh did this get sent to Linus already? I guess I should have reviewed
> this earlier.
Generally, SA11x0 stuff doesn't interest people, and patches I send out
don't attract comments - so I tend to wait a couple of weeks before
queuing them for merging.
It hasn't yet been merged, but is in the queue - arm-soc has taken it
into their late merges, but those haven't yet been sent.
> > > > +};
> > > > +
> > > > +/*
> > > > + * Derived from the table 8-1 in the SA1110 manual, the MPLL appears to
> > > > + * multiply its input rate by 4 x (4 + PPCR). This calculation gives
> > > > + * the exact rate. The figures given in the table are the rates rounded
> > > > + * to 100kHz. Stick with sa11x0_getspeed() for the time being.
> > > [...]
> > > > +static const struct clk_init_data clk_mpll_init_data __initconst = {
> > > > + .name = "mpll",
> > > > + .ops = &clk_mpll_ops,
> > > > + .parent_names = clk_mpll_parents,
> > > > + .num_parents = ARRAY_SIZE(clk_mpll_parents),
> > > > + .flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL,
> > >
> > > Please add a comment about these last two flags so we know why the rate
> > > can't be cached and the clk is critical.
> >
> > Ok, I'll do that with a follow-up patch once the merge window is over.
> >
>
> Ok, thanks.
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-18 22:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-29 10:14 [PATCH] ARM: sa1100: convert to common clock framework Russell King
2019-07-18 16:38 ` Stephen Boyd
2019-07-18 17:49 ` Russell King - ARM Linux admin
2019-07-18 18:43 ` Stephen Boyd
2019-07-18 22:41 ` Russell King - ARM Linux admin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).