Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: "Andreas Färber" <afaerber@suse.de>
To: James Tai <james.tai@realtek.com>
Cc: cylee12 <cylee12@realtek.com>,
	linux-realtek-soc@lists.infradead.org,
	Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
	Paul Walmsley <paul.walmsley@sifive.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-riscv@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/6] clk: realtek: add common clock support for Realtek SoCs
Date: Tue, 3 Dec 2019 16:28:21 +0100
Message-ID: <90124940-e946-1163-8baa-5064d3d973c5@suse.de> (raw)
In-Reply-To: <20191203074513.9416-4-james.tai@realtek.com>

Hi James,

[dropping Palmer]

Am 03.12.19 um 08:45 schrieb James Tai:
> From: cylee12 <cylee12@realtek.com>

Please fix the author name.

> 
> This patch adds common clock support for Realtek SoCs, including PLLs,
> Mux clocks and Gate clocks.

Can you be more specific here, please? Is this compatible back to
RTD1195 or RTD1295 or just 1619 forward? I only see RTD1619 in 5/6. And
not a single .dtsi patch is included in this series for testing it.

> 
> Signed-off-by: Cheng-Yu Lee <cylee12@realtek.com>
> Signed-off-by: James Tai <james.tai@realtek.com>
> ---
>  drivers/clk/Kconfig                   |   1 +
>  drivers/clk/Makefile                  |   1 +
>  drivers/clk/realtek/Kconfig           |  10 +
>  drivers/clk/realtek/Makefile          |   9 +
>  drivers/clk/realtek/clk-pll-dif.c     |  81 ++++++
>  drivers/clk/realtek/clk-pll-psaud.c   | 120 ++++++++
>  drivers/clk/realtek/clk-pll.c         | 400 ++++++++++++++++++++++++++
>  drivers/clk/realtek/clk-pll.h         | 151 ++++++++++
>  drivers/clk/realtek/clk-regmap-gate.c |  89 ++++++
>  drivers/clk/realtek/clk-regmap-gate.h |  26 ++
>  drivers/clk/realtek/clk-regmap-mux.c  |  63 ++++
>  drivers/clk/realtek/clk-regmap-mux.h  |  26 ++
>  drivers/clk/realtek/common.c          | 320 +++++++++++++++++++++
>  drivers/clk/realtek/common.h          | 123 ++++++++
>  14 files changed, 1420 insertions(+)

This patch is way too large for me to review. Please split this up
further into logically self-contained, compile-testable patches.

>  create mode 100644 drivers/clk/realtek/Kconfig
>  create mode 100644 drivers/clk/realtek/Makefile
>  create mode 100644 drivers/clk/realtek/clk-pll-dif.c
>  create mode 100644 drivers/clk/realtek/clk-pll-psaud.c
>  create mode 100644 drivers/clk/realtek/clk-pll.c
>  create mode 100644 drivers/clk/realtek/clk-pll.h
>  create mode 100644 drivers/clk/realtek/clk-regmap-gate.c
>  create mode 100644 drivers/clk/realtek/clk-regmap-gate.h
>  create mode 100644 drivers/clk/realtek/clk-regmap-mux.c
>  create mode 100644 drivers/clk/realtek/clk-regmap-mux.h
>  create mode 100644 drivers/clk/realtek/common.c
>  create mode 100644 drivers/clk/realtek/common.h
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c44247d0b83e..8e06487440ce 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -317,6 +317,7 @@ source "drivers/clk/mediatek/Kconfig"
>  source "drivers/clk/meson/Kconfig"
>  source "drivers/clk/mvebu/Kconfig"
>  source "drivers/clk/qcom/Kconfig"
> +source "drivers/clk/realtek/Kconfig"
>  source "drivers/clk/renesas/Kconfig"
>  source "drivers/clk/samsung/Kconfig"
>  source "drivers/clk/sifive/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 0138fb14e6f8..71ea17f97f7d 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -95,6 +95,7 @@ obj-$(CONFIG_COMMON_CLK_NXP)		+= nxp/
>  obj-$(CONFIG_MACH_PISTACHIO)		+= pistachio/
>  obj-$(CONFIG_COMMON_CLK_PXA)		+= pxa/
>  obj-$(CONFIG_COMMON_CLK_QCOM)		+= qcom/
> +obj-$(CONFIG_COMMON_CLK_REALTEK)	+= realtek/

Should we take the Renesas approach here and allow for COMPILE_TEST?

>  obj-y					+= renesas/
>  obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
>  obj-$(CONFIG_COMMON_CLK_SAMSUNG)	+= samsung/
> diff --git a/drivers/clk/realtek/Kconfig b/drivers/clk/realtek/Kconfig
> new file mode 100644
> index 000000000000..5bca757dddfa
> --- /dev/null
> +++ b/drivers/clk/realtek/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config COMMON_CLK_REALTEK

I would personally think that it's not necessary to prefix with COMMON_
here, even if based on CONFIG_COMMON_CLK framework, since Realtek is no
longer common. Stephen/Michael?

Which brings us to the next aspect, is this really universally common
for Realtek? Are they compatible with old MIPS SoCs and Arm SoCs from
departments other than DHC? (e.g., Smart TV)
If the answer to any of these is no, then please rename the Kconfig
symbol to the oldest SoC for uniqueness, e.g., RTD1195.

> +	bool "Clock driver for realtek"

Spelling.

> +	select MFD_SYSCON

Please add help text and include SoC names.

> +
> +config CLK_PLL_PSAUD

Too generic name.

> +	bool
> +
> +config CLK_PLL_DIF

Ditto.

> +	bool
> diff --git a/drivers/clk/realtek/Makefile b/drivers/clk/realtek/Makefile
> new file mode 100644
> index 000000000000..050d450db067
> --- /dev/null
> +++ b/drivers/clk/realtek/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_COMMON_CLK_REALTEK) += clk-rtk.o
> +
> +clk-rtk-y += common.o
> +clk-rtk-y += clk-regmap-mux.o
> +clk-rtk-y += clk-regmap-gate.o
> +clk-rtk-y += clk-pll.o
> +clk-rtk-$(CONFIG_CLK_PLL_PSAUD) += clk-pll-psaud.o
> +clk-rtk-$(CONFIG_CLK_PLL_DIF) += clk-pll-dif.o
> diff --git a/drivers/clk/realtek/clk-pll-dif.c b/drivers/clk/realtek/clk-pll-dif.c
> new file mode 100644
> index 000000000000..d19efef2626e
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll-dif.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0-only

Can you relicense this code as GPL-2.0-or-later? For Makefile and
Kconfig it doesn't matter, just for low-level code that could become
relevant for debuggers like OpenOCD, which is licensed GPLv2+.

> +/*

Care to elaborate here what "dif" is? :)

> + * Copyright (c) 2019 Realtek Semiconductor Corporation
> + * Author: Cheng-Yu Lee <cylee12@realtek.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>
> +#include <linux/spinlock.h>
> +#include <linux/delay.h>

Insert white line here?

> +#include "common.h"
> +#include "clk-pll.h"
> +
> +static int clk_pll_dif_enable(struct clk_hw *hw)
> +{
> +	struct clk_pll_dif *pll = to_clk_pll_dif(hw);
> +
> +	pr_debug("%pC: %s\n", hw->clk, __func__);

Please review debug and info messages for whether they are still needed
- in particular for info below I assume no.

> +
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x0C, 0x00000048);
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x08, 0x00020c00);
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x04, 0x204004ca);
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x00, 0x8000a000);

This is way too much dark magic!

Start with giving the offsets symbolic names, please.

Next, construct the value from symbolic constants. You will see me use
BIT() and GENMASK() macros elsewhere; please adopt those conventions.

> +	udelay(100);

Is this from some internal datasheet? Otherwise, from some MCU clocks
that I've previously implemented, I would expect there to be some status
bit indicating readyness that we should poll rather than trusting a
hardcoded delay. I don't see a single read below, nor any explanatory
comment.

> +
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x08, 0x00420c00);
> +	udelay(50);
> +
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x08, 0x00420c03);
> +	udelay(200);
> +
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x0C, 0x00000078);
> +	udelay(100);
> +
> +	clk_regmap_write(&pll->clkr, pll->pll_ofs + 0x04, 0x204084ca);
> +
> +	/* ssc control */

This lonely comment seems kind of redundant with ssc_ofs vs. pll_ofs.

> +	clk_regmap_write(&pll->clkr, pll->ssc_ofs + 0x00, 0x00000004);
> +	clk_regmap_write(&pll->clkr, pll->ssc_ofs + 0x04, 0x00006800);
> +	clk_regmap_write(&pll->clkr, pll->ssc_ofs + 0x0C, 0x00000000);
> +	clk_regmap_write(&pll->clkr, pll->ssc_ofs + 0x10, 0x00000000);
> +	clk_regmap_write(&pll->clkr, pll->ssc_ofs + 0x08, 0x001e1f98);
> +	clk_regmap_write(&pll->clkr, pll->ssc_ofs + 0x00, 0x00000005);
> +	pll->status = 1;
> +
> +	return 0;
> +}
[snip]

I'll stop reviewing here and am waiting for a v2.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)


  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-03  7:45 [PATCH 0/6] arm64: Realtek RTD1619 clock and reset controllers James Tai
2019-12-03  7:45 ` [PATCH 1/6] dt-bindings: clock: add bindings for RTD1619 clocks James Tai
2019-12-03  9:32   ` Andreas Färber
2019-12-04  4:11     ` James Tai
2019-12-04 11:25       ` Andreas Färber
2019-12-03  7:45 ` [PATCH 2/6] dt-bindings: reset: add bindings for rtd1619 reset controls James Tai
2019-12-03  7:45 ` [PATCH 3/6] clk: realtek: add common clock support for Realtek SoCs James Tai
2019-12-03 15:28   ` Andreas Färber [this message]
2019-12-03  7:45 ` [PATCH 4/6] clk: realtek: add reset controller " James Tai
2019-12-03 17:46   ` Andreas Färber
2019-12-03  7:45 ` [PATCH 5/6] clk: realtek: add rtd1619 controllers James Tai
2019-12-03  7:45 ` [PATCH 6/6] dt-bindings: clk: realtek: add rtd1619 clock controller bindings James Tai
2019-12-03 18:55   ` Andreas Färber
  -- strict thread matches above, loose matches on Subject: below --
2019-12-03  7:35 [PATCH 0/6] arm64: Realtek RTD1619 clock and reset controllers James Tai
2019-12-03  7:35 ` [PATCH 3/6] clk: realtek: add common clock support for Realtek SoCs James Tai

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=90124940-e946-1163-8baa-5064d3d973c5@suse.de \
    --to=afaerber@suse.de \
    --cc=cylee12@realtek.com \
    --cc=james.tai@realtek.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-realtek-soc@lists.infradead.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=mturquette@baylibre.com \
    --cc=paul.walmsley@sifive.com \
    --cc=sboyd@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-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/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-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


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