On Thu, Jul 18, 2019 at 09:38:08AM -0700, Stephen Boyd wrote:

Quoting Russell King (2019-06-29 03:14:10)

quoted

Convert sa1100 to use the common clock framework.

Signed-off-by: Russell King <redacted>
---
Please ack; this is part of a larger series.  Thanks.

Just a few minor comments but otherwise looks good to me.

quoted

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?

quoted

 
-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" ?

quoted

        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.

quoted

+};
+
+/*
+ * 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.

[...]

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel