Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Jiaxun Yang <jiaxun.yang@flygoat.com>, linux-mips@vger.kernel.org
Cc: mturquette@baylibre.com, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, robh+dt@kernel.org,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: [PATCH v3 2/3] clk: loongson1: add of support
Date: Wed, 06 Feb 2019 11:16:45 -0800
Message-ID: <154948060595.115909.8140700244360302816@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20190201063540.19636-3-jiaxun.yang@flygoat.com>

Quoting Jiaxun Yang (2019-01-31 22:35:39)
> This patch add of support by split the clk_hw register and
> clkdev register, then handle the of clk_hw via
> of_clk_hw_onecell_get.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Why though? There isn't any motivation here.

> ---
>  drivers/clk/loongson1/clk-loongson1b.c | 197 ++++++++++++++++++++-----
>  drivers/clk/loongson1/clk-loongson1c.c | 164 +++++++++++++++-----
>  drivers/clk/loongson1/clk.c            |  47 +++++-
>  drivers/clk/loongson1/clk.h            |  18 ++-
>  4 files changed, 343 insertions(+), 83 deletions(-)

It just looks like more lines are added.

> 
> diff --git a/drivers/clk/loongson1/clk-loongson1b.c b/drivers/clk/loongson1/clk-loongson1b.c
> index f36a97e993c0..2148df31db15 100644
> --- a/drivers/clk/loongson1/clk-loongson1b.c
> +++ b/drivers/clk/loongson1/clk-loongson1b.c
> @@ -1,10 +1,7 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2012-2016 Zhang, Keguang <keguang.zhang@gmail.com>
> - *
> - * This program is free software; you can redistribute  it and/or modify it
> - * under  the terms of  the GNU General  Public License as published by the
> - * Free Software Foundation;  either version 2 of the  License, or (at your
> - * option) any later version.

Please make SPDX change in another patch.

> + * Copyright (c) 2019 Jiaxun Yang <jiaxun.yang@flygoat.com>
>   */
>  
>  #include <linux/clkdev.h>
> @@ -12,11 +9,41 @@
>  #include <linux/io.h>
>  #include <linux/err.h>
>  
> -#include <loongson1.h>
>  #include "clk.h"
> +#include <dt-bindings/clock/ls1b-clock.h>
> +
> +#define LS1B_CLK_COUNT 9
>  
> -#define OSC            (33 * 1000000)
>  #define DIV_APB                2
> +/* Clock PLL Divisor Register Bits */
> +#define DIV_DC_EN                      BIT(31)
> +#define DIV_DC_RST                     BIT(30)
> +#define DIV_CPU_EN                     BIT(25)
> +#define DIV_CPU_RST                    BIT(24)
> +#define DIV_DDR_EN                     BIT(19)
> +#define DIV_DDR_RST                    BIT(18)
> +#define RST_DC_EN                      BIT(5)
> +#define RST_DC                         BIT(4)
> +#define RST_DDR_EN                     BIT(3)
> +#define RST_DDR                                BIT(2)
> +#define RST_CPU_EN                     BIT(1)
> +#define RST_CPU                                BIT(0)
> +
> +#define DIV_DC_SHIFT                   26
> +#define DIV_CPU_SHIFT                  20
> +#define DIV_DDR_SHIFT                  14
> +
> +#define DIV_DC_WIDTH                   4
> +#define DIV_CPU_WIDTH                  4
> +#define DIV_DDR_WIDTH                  4
> +
> +#define BYPASS_DC_SHIFT                        12
> +#define BYPASS_DDR_SHIFT               10
> +#define BYPASS_CPU_SHIFT               8
> +
> +#define BYPASS_DC_WIDTH                        1
> +#define BYPASS_DDR_WIDTH               1
> +#define BYPASS_CPU_WIDTH               1
>  
>  static DEFINE_SPINLOCK(_lock);
>  
> @@ -25,9 +52,9 @@ static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
>  {
>         u32 pll, rate;
>  
> -       pll = __raw_readl(LS1X_CLK_PLL_FREQ);
> +       pll = __raw_readl(CLK_PLL_FREQ_ADDR);
>         rate = 12 + (pll & GENMASK(5, 0));
> -       rate *= OSC;
> +       rate *= parent_rate;
>         rate >>= 1;
>  
>         return rate;

IS this change related? Maybe think about splitting this out too and
just have a patch that re-does all the registration calls in one patch.

> @@ -120,3 +220,24 @@ void __init ls1x_clk_init(void)
>         clk_hw_register_clkdev(hw, "ls1x-wdt", NULL);
>         clk_hw_register_clkdev(hw, "serial8250", NULL);
>  }
> +
> +static void __init ls1b_clk_of_setup(struct device_node *np)
> +{
> +       struct clk_hw_onecell_data *onecell;
> +       int err;
> +       const char *parent = of_clk_get_parent_name(np, 0);
> +
> +       clk_base = of_iomap(np, 0);
> +
> +       onecell = ls1b_clk_init_hw(parent);
> +       if (!onecell)
> +               pr_err("ls1b-clk: unable to register clk_hw");
> +
> +       err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> +       if (err)
> +               pr_err("ls1b-clk: failed to add DT provider: %d\n", err);
> +
> +       pr_info("ls1b-clk: driver registered");

Please no "I'm alive!" messages (plus it's missing a newline at the
end). At the least, make them pr_debug() level.

> +}
> +
> +CLK_OF_DECLARE(clk_ls1b, "loongson,ls1b-clock", ls1b_clk_of_setup);
> diff --git a/drivers/clk/loongson1/clk.c b/drivers/clk/loongson1/clk.c
> index 983ce9f6edbb..b4dc7dd8e0cd 100644
> --- a/drivers/clk/loongson1/clk.c
> +++ b/drivers/clk/loongson1/clk.c
> @@ -1,14 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2012-2016 Zhang, Keguang <keguang.zhang@gmail.com>
> - *
> - * This program is free software; you can redistribute  it and/or modify it
> - * under  the terms of  the GNU General  Public License as published by the
> - * Free Software Foundation;  either version 2 of the  License, or (at your
> - * option) any later version.
> + * Copyright (c) 2019 Jiaxun Yang <jiaxun.yang@flygoat.com>
>   */
>  
> +#include <linux/clkdev.h>
> +
>  #include <linux/clk-provider.h>
>  #include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +
> +#include <asm/mach-loongson32/platform.h>
> +
> +void __iomem *clk_base;
> +
> +#define LS1C_OSC               (24 * 1000000)
> +#define LS1B_OSC               (33 * 1000000)
> +
> +#define LS1X_CLK_BASE  0x1fe78030
>  
>  #include "clk.h"
>  
> @@ -43,3 +53,30 @@ struct clk_hw *__init clk_hw_register_pll(struct device *dev,
>  
>         return hw;
>  }
> +
> +void __init ls1x_clk_init(void)
> +{
> +       struct clk_hw *hw;
> +       struct clk_hw_onecell_data *onecell;
> +
> +       clk_base = (void __iomem *)KSEG1ADDR(LS1X_CLK_BASE);
> +#ifdef CONFIG_LOONGSON1_LS1B
> +       hw = clk_hw_register_fixed_rate(NULL, "osc_clk", NULL, 0, LS1B_OSC);
> +       clk_hw_register_clkdev(hw, "osc_clk", NULL);
> +       onecell = ls1c_clk_init_hw("osc_clk");
> +       if (!onecell)
> +               panic("ls1x-clk: unable to register clk_hw");
> +
> +       ls1c_register_clkdev(onecell);

Should be ls1b_register_clkdev?

> +#elif defined(CONFIG_LOONGSON1_LS1C)
> +       hw = clk_hw_register_fixed_rate(NULL, "osc_clk", NULL, 0, LS1C_OSC);
> +       clk_hw_register_clkdev(hw, "osc_clk", NULL);
> +       onecell = ls1c_clk_init_hw("osc_clk");
> +       if (!onecell)
> +               panic("ls1x-clk: unable to register clk_hw");
> +
> +       ls1c_register_clkdev(onecell);
> +#else
> +       panic("ls1x-clk: not loongson platform");
> +#endif

Can this be implemented with if (IS_ENABLED(CONFIG_LOONGSON1_LS1B))
else-if, etc.? That would make the maybe unused attribute unnecessary and
provide more compile time coverage.

Also, a lot of the code is duplicated twice so that could all be
collapsed together and then the only difference is the fixed rate and
the registration function at the end? 

Or can this just be implemented in the 1b and 1c files instead of in
some common place here?

> +}
> diff --git a/drivers/clk/loongson1/clk.h b/drivers/clk/loongson1/clk.h
> index 085d74b5d496..be7cc72c0ea3 100644
> --- a/drivers/clk/loongson1/clk.h
> +++ b/drivers/clk/loongson1/clk.h
> @@ -16,4 +13,15 @@ struct clk_hw *clk_hw_register_pll(struct device *dev,
>                                    const struct clk_ops *ops,
>                                    unsigned long flags);
>  
> +struct clk_hw_onecell_data __init *ls1c_clk_init_hw(const char *osc_name);
> +struct clk_hw_onecell_data __init *ls1b_clk_init_hw(const char *osc_name);
> +
> +void __maybe_unused __init ls1c_register_clkdev(struct clk_hw_onecell_data *onecell);
> +void __maybe_unused __init ls1b_register_clkdev(struct clk_hw_onecell_data *onecell);

I doubt that __maybe_unused and __init are needed here in the header
file. Just on the actual implementation of the function.


  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-25 13:34 [PATCH 0/3] Enhance loongson-1 clock driver Jiaxun Yang
2019-01-25 13:34 ` [PATCH 1/3] clk: loongson1: add configuration option for loongson1 clks Jiaxun Yang
2019-01-25 13:34 ` [PATCH 2/3] clk: loongson1: add of support Jiaxun Yang
2019-01-25 13:34 ` [PATCH 3/3] dt-bindings: clock: Add loongson-1 clock bindings Jiaxun Yang
2019-01-28 15:20 ` [PATCH 0/3] Enhance loongson-1 clock driver Jiaxun Yang
2019-01-28 15:20   ` [PATCH v2 1/3] clk: loongson1: add configuration option for loongson1 clks Jiaxun Yang
2019-01-28 15:20   ` [PATCH v2 2/3] clk: loongson1: add of support Jiaxun Yang
2019-01-28 15:20   ` [PATCH v2 3/3] dt-bindings: clock: Add loongson-1 clock bindings Jiaxun Yang
2019-01-30 19:47     ` Rob Herring
2019-02-01  6:35   ` [PATCH v3 0/3] Enhance loongson-1 clock driver Jiaxun Yang
2019-02-01  6:35     ` [PATCH v3 1/3] clk: loongson1: add configuration option for loongson1 clks Jiaxun Yang
2019-02-06 19:18       ` Stephen Boyd
2019-02-01  6:35     ` [PATCH v3 2/3] clk: loongson1: add of support Jiaxun Yang
2019-02-06 19:16       ` Stephen Boyd [this message]
2019-02-01  6:35     ` [PATCH v3 3/3] dt-bindings: clock: Add loongson-1 clock bindings Jiaxun Yang
2019-02-13 22:56       ` Rob Herring

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=154948060595.115909.8140700244360302816@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jiaxun.yang@flygoat.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox