From: Heiko Stuebner <heiko@sntech.de>
To: Andy Yan <andy.yan@rock-chips.com>
Cc: elaine.zhang@rock-chips.com, mturquette@baylibre.com,
linux-rockchip@lists.infradead.org, sboyd@codeaurora.org,
linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
Shawn Lin <shawn.lin@rock-chips.com>
Subject: Re: [PATCH 3/6] clk: rockchip: add clock controller for rk1108
Date: Fri, 04 Nov 2016 08:32:15 +0100 [thread overview]
Message-ID: <4415767.udKEaOCZjv@phil> (raw)
In-Reply-To: <1478176713-12066-1-git-send-email-andy.yan@rock-chips.com>
Hi Andy,
Am Donnerstag, 3. November 2016, 20:38:33 CET schrieb Andy Yan:
> From: Shawn Lin <shawn.lin@rock-chips.com>
>
> Add the clock tree definition and driver for rk1108 SoC.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
>
> drivers/clk/rockchip/Makefile | 1 +
> drivers/clk/rockchip/clk-rk1108.c | 463
> +++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.h |
> 14 +
> include/dt-bindings/clock/rk1108-cru.h | 308 ++++++++++++++++++++++
Please split the rk1108-cru.h addition into a separate patch, so that I can
put it into a shared branch for clock and dts branches.
Also it looks like you didn't provide a devicetree binding document (in a
separate patch).
Clock-tree looks mostly good, just some small things below
> 4 files changed, 786 insertions(+)
> create mode 100644 drivers/clk/rockchip/clk-rk1108.c
> create mode 100644 include/dt-bindings/clock/rk1108-cru.h
>
> diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
> index b5f2c8e..16e098c 100644
> --- a/drivers/clk/rockchip/Makefile
> +++ b/drivers/clk/rockchip/Makefile
> @@ -11,6 +11,7 @@ obj-y += clk-mmc-phase.o
> obj-y += clk-ddr.o
> obj-$(CONFIG_RESET_CONTROLLER) += softrst.o
>
> +obj-y += clk-rk1108.o
> obj-y += clk-rk3036.o
> obj-y += clk-rk3188.o
> obj-y += clk-rk3228.o
> diff --git a/drivers/clk/rockchip/clk-rk1108.c
> b/drivers/clk/rockchip/clk-rk1108.c new file mode 100644
> index 0000000..eafc623
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-rk1108.c
> @@ -0,0 +1,463 @@
> +/*
> + * Copyright (c) 2016 Rockchip Electronics Co. Ltd.
> + * Author: Shawn Lin <shawn.lin@rock-chips.com>
> + * Andy Yan <andy.yan@rock-chips.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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/syscore_ops.h>
> +#include <dt-bindings/clock/rk1108-cru.h>
> +#include "clk.h"
> +
> +#define RK1108_GRF_SOC_STATUS0 0x480
> +
> +enum rk1108_plls {
> + apll, dpll, gpll,
> +};
> +
> +static struct rockchip_pll_rate_table rk1108_pll_rates[] = {
> + /* _mhz, _refdiv, _fbdiv, _postdiv1, _postdiv2, _dsmpd, _frac */
> + RK3036_PLL_RATE(1608000000, 1, 67, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1584000000, 1, 66, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1560000000, 1, 65, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1536000000, 1, 64, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1512000000, 1, 63, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1488000000, 1, 62, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1464000000, 1, 61, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1440000000, 1, 60, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1416000000, 1, 59, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1392000000, 1, 58, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1368000000, 1, 57, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1344000000, 1, 56, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1320000000, 1, 55, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1296000000, 1, 54, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1272000000, 1, 53, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1248000000, 1, 52, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1200000000, 1, 50, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1188000000, 2, 99, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1104000000, 1, 46, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1100000000, 12, 550, 1, 1, 1, 0),
> + RK3036_PLL_RATE(1008000000, 1, 84, 2, 1, 1, 0),
> + RK3036_PLL_RATE(1000000000, 6, 500, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 984000000, 1, 82, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 960000000, 1, 80, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 936000000, 1, 78, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 912000000, 1, 76, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 900000000, 4, 300, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 888000000, 1, 74, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 864000000, 1, 72, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 840000000, 1, 70, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 816000000, 1, 68, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 800000000, 6, 400, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 700000000, 6, 350, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 696000000, 1, 58, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 600000000, 1, 75, 3, 1, 1, 0),
> + RK3036_PLL_RATE( 594000000, 2, 99, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 504000000, 1, 63, 3, 1, 1, 0),
> + RK3036_PLL_RATE( 500000000, 6, 250, 2, 1, 1, 0),
> + RK3036_PLL_RATE( 408000000, 1, 68, 2, 2, 1, 0),
> + RK3036_PLL_RATE( 312000000, 1, 52, 2, 2, 1, 0),
> + RK3036_PLL_RATE( 216000000, 1, 72, 4, 2, 1, 0),
> + RK3036_PLL_RATE( 96000000, 1, 64, 4, 4, 1, 0),
> + { /* sentinel */ },
> +};
> +
> +
double empty line. There are a lot more in this file, so please remove all of
them. One line of space between blocks is enough :-) .
[...]
> diff --git a/include/dt-bindings/clock/rk1108-cru.h
> b/include/dt-bindings/clock/rk1108-cru.h new file mode 100644
> index 0000000..e731cc8
> --- /dev/null
> +++ b/include/dt-bindings/clock/rk1108-cru.h
> @@ -0,0 +1,308 @@
> +/*
> + * Copyright (c) 2016 Rockchip Electronics Co. Ltd.
> + * Author:
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_ROCKCHIP_RK1108_H
> +#define _DT_BINDINGS_CLK_ROCKCHIP_RK1108_H
> +
> +/* pll id */
> +#define RK1108_APLL_ID 0
> +#define RK1108_DPLL_ID 1
> +#define RK1108_GPLL_ID 2
> +#define RK1108_ARMCLK 3
> +#define RK1108_END_PLL_ID 4
what is this supposed to do. Looks like it should go away.
> +
> +/* sclk gates (special clocks) */
> +#define SCLK_SPI0 65
> +#define SCLK_NANDC 67
> +#define SCLK_SDMMC 68
> +#define SCLK_SDIO 69
> +#define SCLK_EMMC 71
> +#define SCLK_UART0 72
> +#define SCLK_UART1 73
> +#define SCLK_UART2 74
> +#define SCLK_I2S0 75
> +#define SCLK_I2S1 76
> +#define SCLK_I2S2 77
> +#define SCLK_TIMER0 78
> +#define SCLK_TIMER1 79
> +#define SCLK_SFC 80
> +#define SCLK_SDMMC_DRV 81
> +#define SCLK_SDIO_DRV 82
> +#define SCLK_EMMC_DRV 83
> +#define SCLK_SDMMC_SAMPLE 84
> +#define SCLK_SDIO_SAMPLE 85
> +#define SCLK_EMMC_SAMPLE 86
> +
> +/* aclk gates */
> +#define ACLK_DMAC 251
> +#define ACLK_PRE 252
> +#define ACLK_CORE 253
> +#define ACLK_ENMCORE 254
> +
> +/* pclk gates */
> +#define PCLK_GPIO1 321
> +#define PCLK_GPIO2 322
> +#define PCLK_GPIO3 323
> +#define PCLK_GRF 329
> +#define PCLK_I2C1 333
> +#define PCLK_I2C2 334
> +#define PCLK_I2C3 335
> +#define PCLK_SPI 338
> +#define PCLK_SFC 339
> +#define PCLK_UART0 341
> +#define PCLK_UART1 342
> +#define PCLK_UART2 343
> +#define PCLK_TSADC 344
> +#define PCLK_PWM 350
> +#define PCLK_TIMER 353
> +#define PCLK_PERI 363
> +
> +/* hclk gates */
> +#define HCLK_I2S0_8CH 442
> +#define HCLK_I2S1_8CH 443
> +#define HCLK_I2S2_2CH 444
> +#define HCLK_NANDC 453
> +#define HCLK_SDMMC 456
> +#define HCLK_SDIO 457
> +#define HCLK_EMMC 459
> +#define HCLK_PERI 478
> +#define HCLK_SFC 479
> +
> +#define CLK_NR_CLKS (HCLK_SFC + 1)
> +
> +/* reset id */
> +#define SRST_CORE_PO_AD 0
> +#define SRST_CORE_AD 1
> +#define SRST_L2_AD 2
> +#define SRST_CPU_NIU_AD 3
> +#define SRST_CORE_PO 4
> +#define SRST_CORE 5
> +#define SRST_L2 6
> +#define RST_0RES7 7
> +#define SRST_CORE_DBG 8
> +#define PRST_DBG 9
> +#define RST_DAP 10
> +#define PRST_DBG_NIU 11
> +#define RST_0RES12 12
> +#define RST_0RES13 13
> +#define RST_0RES14 14
> +#define ARST_STRC_SYS_AD 15
> +
> +#define SRST_DDRPHY_CLKDIV 16
> +#define SRST_DDRPHY 17
> +#define PRST_DDRPHY 18
> +#define PRST_HDMIPHY 19
> +#define PRST_VDACPHY 20
> +#define PRST_VADCPHY 21
> +#define PRST_MIPI_CSI_PHY 22
> +#define PRST_MIPI_DSI_PHY 23
> +#define PRST_ACODEC 24
> +#define ARST_BUS_NIU 25
> +#define PRST_TOP_NIU 26
> +#define ARST_INTMEM 27
> +#define HRST_ROM 28
> +#define ARST_DMAC 29
> +#define SRST_MSCH_NIU 30
> +#define PRST_MSCH_NIU 31
> +
> +#define PRST_DDRUPCTL 32
> +#define NRST_DDRUPCTL 33
> +#define PRST_DDRMON 34
> +#define HRST_I2S0_8CH 35
> +#define MRST_I2S0_8CH 36
> +#define HRST_I2S1_2CH 37
> +#define MRST_IS21_2CH 38
> +#define HRST_I2S2_2CH 39
> +#define MRST_I2S2_2CH 40
> +#define HRST_CRYPTO 41
> +#define SRST_CRYPTO 42
> +#define PRST_SPI 43
> +#define SRST_SPI 44
> +#define PRST_UART0 45
> +#define PRST_UART1 46
> +#define PRST_UART2 47
> +
> +#define SRST_UART0 48
> +#define SRST_UART1 49
> +#define SRST_UART2 50
> +#define PRST_I2C1 51
> +#define PRST_I2C2 52
> +#define PRST_I2C3 53
> +#define SRST_I2C1 54
> +#define SRST_I2C2 55
> +#define SRST_I2C3 56
> +#define RST_3RES9 57
> +#define PRST_PWM1 58
> +#define RST_3RES11 59
> +#define SRST_PWM1 60
> +#define PRST_WDT 61
> +#define PRST_GPIO1 62
> +#define PRST_GPIO2 63
> +
> +#define PRST_GPIO3 64
> +#define PRST_GRF 65
> +#define PRST_EFUSE 66
> +#define PRST_EFUSE512 67
> +#define PRST_TIMER0 68
> +#define SRST_TIMER0 69
> +#define SRST_TIMER1 70
> +#define PRST_TSADC 71
> +#define SRST_TSADC 72
> +#define PRST_SARADC 73
> +#define SRST_SARADC 74
> +#define HRST_SYSBUS 75
> +#define PRST_USBGRF 76
> +#define RST_4RES13 77
> +#define RST_4RES14 78
> +#define RST_4RES15 79
> +
> +#define ARST_PERIPH_NIU 80
> +#define HRST_PERIPH_NIU 81
> +#define PRST_PERIPH_NIU 82
> +#define HRST_PERIPH 83
> +#define HRST_SDMMC 84
> +#define HRST_SDIO 85
> +#define HRST_EMMC 86
> +#define HRST_NANDC 87
> +#define NRST_NANDC 88
> +#define HRST_SFC 89
> +#define SRST_SFC 90
> +#define ARST_GMAC 91
> +#define HRST_OTG 92
> +#define SRST_OTG 93
> +#define SRST_OTG_ADP 94
> +#define HRST_HOST0 95
> +
> +#define HRST_HOST0_AUX 96
> +#define HRST_HOST0_ARB 97
> +#define SRST_HOST0_EHCIPHY 98
> +#define SRST_HOST0_UTMI 99
> +#define SRST_USBPOR 100
> +#define SRST_UTMI0 101
> +#define SRST_UTMI1 102
> +#define RST_6RES7 103
> +#define RST_6RES8 104
> +#define RST_6RES9 105
> +#define RST_6RES10 106
> +#define RST_6RES11 107
> +#define RST_6RES12 108
> +#define RST_6RES13 109
> +#define RST_6RES14 110
> +#define RST_6RES15 101
> +
> +#define ARST_VIO0_NIU 102
> +#define ARST_VIO1_NIU 103
> +#define HRST_VIO_NIU 104
> +#define PRST_VIO_NIU 105
> +#define ARST_VOP 106
> +#define HRST_VOP 107
> +#define DRST_VOP 108
> +#define ARST_IEP 109
> +#define HRST_IEP 110
> +#define ARST_RGA 111
> +#define HRST_RGA 112
> +#define SRST_RGA 113
> +#define PRST_CVBS 114
> +#define PRST_HDMI 115
> +#define SRST_HDMI 116
> +#define PRST_MIPI_DSI 117
> +
> +#define ARST_ISP_NIU 118
> +#define HRST_ISP_NIU 119
> +#define HRST_ISP 120
> +#define SRST_ISP 121
> +#define ARST_VIP0 122
> +#define HRST_VIP0 123
> +#define PRST_VIP0 124
> +#define ARST_VIP1 125
> +#define HRST_VIP1 126
> +#define PRST_VIP1 127
> +#define ARST_VIP2 128
> +#define HRST_VIP2 129
> +#define PRST_VIP2 120
> +#define ARST_VIP3 121
> +#define HRST_VIP3 122
> +#define PRST_VIP4 123
> +
> +#define PRST_CIF1TO4 124
> +#define SRST_CVBS_CLK 125
> +#define HRST_CVBS 126
> +#define RST_9RES3 127
> +#define RST_9RES4 128
> +#define RST_9RES5 129
> +#define RST_9RES6 130
> +#define RST_9RES7 131
> +#define RST_9RES8 132
> +#define RST_9RES9 133
> +#define RST_9RES10 134
> +#define RST_9RES11 134
> +#define RST_9RES12 136
> +#define RST_9RES13 137
> +#define RST_9RES14 138
> +#define RST_9RES15 139
there is no need to document unused/reserved bits in the header,
so please drop all of them (more in the areas above here as well).
Heiko
next prev parent reply other threads:[~2016-11-04 7:32 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-03 12:26 [PATCH 0/6] Add basic support for support for Rockchip RK1108 SOC Andy Yan
2016-11-03 12:30 ` [PATCH 1/6] dt-bindings: rockchip-dw-mshc: add RK1108 dw-mshc description Andy Yan
2016-11-10 18:56 ` Rob Herring
2016-11-03 12:34 ` [PATCH 2/6] pinctrl: rockchip: add support for rk1108 Andy Yan
2016-11-03 15:55 ` Heiko Stübner
2016-11-06 10:05 ` Linus Walleij
2016-11-12 17:41 ` 陈豪
2016-11-12 21:44 ` Heiko Stübner
2016-11-13 7:24 ` Andy Yan
2016-11-03 12:38 ` [PATCH 3/6] clk: rockchip: add clock controller " Andy Yan
2016-11-04 2:09 ` Shawn Lin
2016-11-04 7:32 ` Heiko Stuebner [this message]
2016-11-03 12:40 ` [PATCH 4/6] ARM: dts: add basic support for Rockchip RK1108 SOC Andy Yan
2016-11-04 8:00 ` Heiko Stuebner
2016-11-08 12:31 ` Andy Yan
2016-11-08 13:20 ` Heiko Stübner
2016-11-04 8:07 ` Heiko Stuebner
2016-11-03 12:42 ` [PATCH 5/6] ARM: add low level debug uart for rk1108 Andy Yan
2016-11-04 7:36 ` Heiko Stuebner
2016-11-04 7:58 ` Andy Yan
2016-11-04 8:03 ` Heiko Stuebner
2016-11-03 12:43 ` [PATCH 6/6] ARM: dts: rockchip: add rockchip RK1108 Evaluation board Andy Yan
2016-11-04 10:03 ` Heiko Stuebner
2016-11-04 10:54 ` Andy Yan
2016-11-04 11:17 ` Heiko Stuebner
2016-11-10 18:57 ` Rob Herring
2016-11-12 16:02 ` [PATCH 0/6] Add basic support for support for Rockchip RK1108 SOC 陈豪
Reply instructions:
You may reply publicly 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=4415767.udKEaOCZjv@phil \
--to=heiko@sntech.de \
--cc=andy.yan@rock-chips.com \
--cc=elaine.zhang@rock-chips.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mturquette@baylibre.com \
--cc=sboyd@codeaurora.org \
--cc=shawn.lin@rock-chips.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).