On Wed, 13 Sep 2017, David Wu wrote:

The ADC can support some channels signal-ended some bits Successive Approximation Register (SAR) A/D Converter, like 6-channel and 10-bit. It converts the analog input signal into some bits binary digital codes.

Signed-off-by: David Wu david.wu@rock-chips.com

Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

Please see below for requested changes.

---

drivers/adc/Kconfig | 9 ++ drivers/adc/Makefile | 1 + drivers/adc/rockchip-saradc.c | 188 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 drivers/adc/rockchip-saradc.c

diff --git a/drivers/adc/Kconfig b/drivers/adc/Kconfig index e5335f7..830fe0f 100644 --- a/drivers/adc/Kconfig +++ b/drivers/adc/Kconfig @@ -20,6 +20,15 @@ config ADC_EXYNOS - 12-bit resolution - 600 KSPS of sample rate

+config SARADC_ROCKCHIP

• bool "Enable Rockchip SARADC driver"
• help

•  This enables driver for Rockchip SARADC.
  

•  It provides:
  

•  - 2~6 analog input channels
  

•  - 1O-bit resolution
  

•  - 1MSPS of sample rate
  

config ADC_SANDBOX bool "Enable Sandbox ADC test driver" help diff --git a/drivers/adc/Makefile b/drivers/adc/Makefile index cebf26d..4b5aa69 100644 --- a/drivers/adc/Makefile +++ b/drivers/adc/Makefile @@ -8,3 +8,4 @@ obj-$(CONFIG_ADC) += adc-uclass.o obj-$(CONFIG_ADC_EXYNOS) += exynos-adc.o obj-$(CONFIG_ADC_SANDBOX) += sandbox.o +obj-$(CONFIG_SARADC_ROCKCHIP) += rockchip-saradc.o

Do you feel strongly about the "SARADC_ROCKCHIP" or would "ADC_ROCKCHIP" be correct as well? I don't care either way, but this is the first entry here that does not start with CONFIG_ADC_, so I am wondering...

diff --git a/drivers/adc/rockchip-saradc.c b/drivers/adc/rockchip-saradc.c new file mode 100644 index 0000000..5c7c3d9 --- /dev/null +++ b/drivers/adc/rockchip-saradc.c @@ -0,0 +1,188 @@ +/*

• • (C) Copyright 2017, Fuzhou Rockchip Electronics Co., Ltd
• • SPDX-License-Identifier: GPL-2.0+
• • Rockchip Saradc driver for U-Boot

Should this be SARADC (all uppercase)?

• */

+#include <asm/io.h> +#include <clk.h> +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <adc.h>

Please see https://www.denx.de/wiki/U-Boot/CodingStyle for the include order. Please revise.

@Simon: yes, I am quick study ;-)

+#define SARADC_DATA 0x00

+#define SARADC_STAS 0x04

Please see https://www.denx.de/wiki/U-Boot/CodingStyle for the guidelines on using structures for I/O accesses. Please revise.

+#define SARADC_STAS_BUSY BIT(0)

+#define SARADC_CTRL 0x08 +#define SARADC_CTRL_POWER_CTRL BIT(3) +#define SARADC_CTRL_CHN_MASK 0x7 +#define SARADC_CTRL_IRQ_STATUS BIT(6) +#define SARADC_CTRL_IRQ_ENABLE BIT(5)

+#define SARADC_DLY_PU_SOC 0x0c

+#define SARADC_TIMEOUT (100 * 1000)

+struct rockchip_saradc_data {

• int num_bits;
• int num_channels;
• unsigned long clk_rate;

+};

+struct rockchip_saradc_priv {

• fdt_addr_t regs;
• int active_channel;
• const struct rockchip_saradc_data *data;

+};

+int rockchip_saradc_channel_data(struct udevice *dev, int channel,

• 	    unsigned int *data)
  

+{

• struct rockchip_saradc_priv *priv = dev_get_priv(dev);
• if (channel != priv->active_channel) {

• error("Requested channel is not active!");
  

• return -EINVAL;
  

• }
• if ((readl(priv->regs + SARADC_CTRL) & SARADC_CTRL_IRQ_STATUS) != SARADC_CTRL_IRQ_STATUS)

• return -EBUSY;
  

• /* Read value */
• *data = readl(priv->regs + SARADC_DATA);
• *data &= (1 << priv->data->num_bits) - 1;

This recomputes the data_mask (from below). Can we just use the data_mask again?

• /* Power down adc */
• writel(0, priv->regs + SARADC_CTRL);
• return 0;

+}

+int rockchip_saradc_start_channel(struct udevice *dev, int channel) +{

• struct rockchip_saradc_priv *priv = dev_get_priv(dev);
• if (channel < 0 || channel >= priv->data->num_channels) {

• error("Requested channel is invalid!");
  

• return -EINVAL;
  

• }
• /* 8 clock periods as delay between power up and start cmd */
• writel(8, priv->regs + SARADC_DLY_PU_SOC);
• /* Select the channel to be used and trigger conversion */
• writel(SARADC_CTRL_POWER_CTRL

• 	| (channel & SARADC_CTRL_CHN_MASK) | SARADC_CTRL_IRQ_ENABLE,
  

This line does not match the style guideline: it is too wide and the operator should be before the line-break.

Did you run checkpatch or use patman?

•    priv->regs + SARADC_CTRL);
  

• priv->active_channel = channel;
• return 0;

+}

+int rockchip_saradc_stop(struct udevice *dev) +{

• struct rockchip_saradc_priv *priv = dev_get_priv(dev);
• /* Power down adc */
• writel(0, priv->regs + SARADC_CTRL);
• priv->active_channel = -1;
• return 0;

+}

+int rockchip_saradc_probe(struct udevice *dev) +{

• struct rockchip_saradc_priv *priv = dev_get_priv(dev);
• struct clk clk;
• int ret;
• ret = clk_get_by_index(dev, 0, &clk);
• if (ret)

• return ret;
  

• ret = clk_set_rate(&clk, priv->data->clk_rate);
• if (IS_ERR_VALUE(ret))

• return ret;
  

• priv->active_channel = -1;
• return 0;

+}

+int rockchip_saradc_ofdata_to_platdata(struct udevice *dev) +{

• struct adc_uclass_platdata *uc_pdata = dev_get_uclass_platdata(dev);
• struct rockchip_saradc_priv *priv = dev_get_priv(dev);
• struct rockchip_saradc_data *data =

• 			(struct rockchip_saradc_data *)dev_get_driver_data(dev);
  

• priv->regs = devfdt_get_addr(dev);

Shouldn't this be dev_read_addr?

• if (priv->regs == FDT_ADDR_T_NONE) {

• error("Dev: %s - can't get address!", dev->name);
  

• return -ENODATA;
  

• }
• priv->data = data;
• uc_pdata->data_mask = (1 << priv->data->num_bits) - 1;;
• uc_pdata->data_format = ADC_DATA_FORMAT_BIN;
• uc_pdata->data_timeout_us = SARADC_TIMEOUT / 5;
• uc_pdata->channel_mask = (1 << priv->data->num_channels) - 1;
• return 0;

+}

+static const struct adc_ops rockchip_saradc_ops = {

• .start_channel = rockchip_saradc_start_channel,
• .channel_data = rockchip_saradc_channel_data,
• .stop = rockchip_saradc_stop,

+};

+static const struct rockchip_saradc_data saradc_data = {

• .num_bits = 10,
• .num_channels = 3,
• .clk_rate = 1000000,

+};

+static const struct rockchip_saradc_data rk3066_tsadc_data = {

• .num_bits = 12,
• .num_channels = 2,
• .clk_rate = 50000,

+};

+static const struct rockchip_saradc_data rk3399_saradc_data = {

• .num_bits = 10,
• .num_channels = 6,
• .clk_rate = 1000000,

+};

+static const struct udevice_id rockchip_saradc_ids[] = {

• {

• .compatible = "rockchip,saradc",
  

• .data = (ulong)&saradc_data,
  

• },
• {

• .compatible = "rockchip,rk3066-tsadc",
  

• .data = (ulong)&rk3066_tsadc_data,
  

• }, {

• .compatible = "rockchip,rk3399-saradc",
  

• .data = (ulong)&rk3399_saradc_data,
  

• },
• { }

+};

+U_BOOT_DRIVER(rockchip_saradc) = {

• .name = "rockchip_saradc",
• .id = UCLASS_ADC,
• .of_match = rockchip_saradc_ids,
• .ops = &rockchip_saradc_ops,
• .probe = rockchip_saradc_probe,
• .ofdata_to_platdata = rockchip_saradc_ofdata_to_platdata,
• .priv_auto_alloc_size = sizeof(struct rockchip_saradc_priv),

+};

On 13 Sep 2017, at 12:09, David Wu david.wu@rock-chips.com wrote:

Except for 3036 and 3228 Socs, which don't support Saradc, enable the ROCKCHIP_SARADC config at the other Socs' defconfig.

Please use an ‘imply’ in the Kconfig for the various SoCs (or possibly even the entire sub-architecture) or a "default y if …”.

Given that this is a DM-enabled driver, there is no harm in compiling it in, even if a given board does not use it. This should make defconfig changes unnecessary (unless someone really needs to override the implied setting).

Signed-off-by: David Wu david.wu@rock-chips.com

configs/evb-rk3288_defconfig | 2 ++ configs/evb-rk3328_defconfig | 2 ++ configs/evb-rk3399_defconfig | 2 ++ configs/evb-rv1108_defconfig | 2 ++ configs/fennec-rk3288_defconfig | 2 ++ configs/firefly-rk3288_defconfig | 2 ++ configs/firefly-rk3399_defconfig | 2 ++ configs/lion-rk3368_defconfig | 2 ++ configs/miqi-rk3288_defconfig | 2 ++ configs/phycore-rk3288_defconfig | 2 ++ configs/popmetal-rk3288_defconfig | 2 ++ configs/puma-rk3399_defconfig | 2 ++ configs/sheep-rk3368_defconfig | 2 ++ configs/tinker-rk3288_defconfig | 2 ++ 14 files changed, 28 insertions(+)

diff --git a/configs/evb-rk3288_defconfig b/configs/evb-rk3288_defconfig index 5294ba9..f09b769 100644 --- a/configs/evb-rk3288_defconfig +++ b/configs/evb-rk3288_defconfig @@ -37,6 +37,8 @@ CONFIG_REGMAP=y CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/evb-rk3328_defconfig b/configs/evb-rk3328_defconfig index 7bec001..b44b029 100644 --- a/configs/evb-rk3328_defconfig +++ b/configs/evb-rk3328_defconfig @@ -20,6 +20,8 @@ CONFIG_CMD_TIME=y CONFIG_ENV_IS_IN_MMC=y CONFIG_REGMAP=y CONFIG_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_ROCKCHIP_GPIO=y CONFIG_MMC_DW=y diff --git a/configs/evb-rk3399_defconfig b/configs/evb-rk3399_defconfig index 7a0bd4a..6d0d1a0 100644 --- a/configs/evb-rk3399_defconfig +++ b/configs/evb-rk3399_defconfig @@ -30,6 +30,8 @@ CONFIG_REGMAP=y CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/evb-rv1108_defconfig b/configs/evb-rv1108_defconfig index ab4276a..3278104 100644 --- a/configs/evb-rv1108_defconfig +++ b/configs/evb-rv1108_defconfig @@ -13,6 +13,8 @@ CONFIG_CMD_TIME=y CONFIG_NET_RANDOM_ETHADDR=y CONFIG_REGMAP=y CONFIG_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_ROCKCHIP_GPIO=y CONFIG_SYS_I2C_ROCKCHIP=y diff --git a/configs/fennec-rk3288_defconfig b/configs/fennec-rk3288_defconfig index 96a07de..913849e 100644 --- a/configs/fennec-rk3288_defconfig +++ b/configs/fennec-rk3288_defconfig @@ -40,6 +40,8 @@ CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y # CONFIG_SPL_SIMPLE_BUS is not set +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/firefly-rk3288_defconfig b/configs/firefly-rk3288_defconfig index 82da601..75f8cdb 100644 --- a/configs/firefly-rk3288_defconfig +++ b/configs/firefly-rk3288_defconfig @@ -40,6 +40,8 @@ CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y # CONFIG_SPL_SIMPLE_BUS is not set +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/firefly-rk3399_defconfig b/configs/firefly-rk3399_defconfig index 94b9209..e9e4324 100644 --- a/configs/firefly-rk3399_defconfig +++ b/configs/firefly-rk3399_defconfig @@ -30,6 +30,8 @@ CONFIG_REGMAP=y CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/lion-rk3368_defconfig b/configs/lion-rk3368_defconfig index 45a12a8..5ef6d4b 100644 --- a/configs/lion-rk3368_defconfig +++ b/configs/lion-rk3368_defconfig @@ -52,6 +52,8 @@ CONFIG_TPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y CONFIG_TPL_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_TPL_CLK=y diff --git a/configs/miqi-rk3288_defconfig b/configs/miqi-rk3288_defconfig index b0437e1..7825467 100644 --- a/configs/miqi-rk3288_defconfig +++ b/configs/miqi-rk3288_defconfig @@ -40,6 +40,8 @@ CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y # CONFIG_SPL_SIMPLE_BUS is not set +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/phycore-rk3288_defconfig b/configs/phycore-rk3288_defconfig index 93ee353..911600d 100644 --- a/configs/phycore-rk3288_defconfig +++ b/configs/phycore-rk3288_defconfig @@ -42,6 +42,8 @@ CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y # CONFIG_SPL_SIMPLE_BUS is not set +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/popmetal-rk3288_defconfig b/configs/popmetal-rk3288_defconfig index 5e99f9c..9f5d78d 100644 --- a/configs/popmetal-rk3288_defconfig +++ b/configs/popmetal-rk3288_defconfig @@ -40,6 +40,8 @@ CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y # CONFIG_SPL_SIMPLE_BUS is not set +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/puma-rk3399_defconfig b/configs/puma-rk3399_defconfig index 1badf80..7929a69 100644 --- a/configs/puma-rk3399_defconfig +++ b/configs/puma-rk3399_defconfig @@ -41,6 +41,8 @@ CONFIG_REGMAP=y CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y diff --git a/configs/sheep-rk3368_defconfig b/configs/sheep-rk3368_defconfig index b862a14..d4877d3 100644 --- a/configs/sheep-rk3368_defconfig +++ b/configs/sheep-rk3368_defconfig @@ -10,6 +10,8 @@ CONFIG_ANDROID_BOOT_IMAGE=y CONFIG_CMD_MMC=y CONFIG_REGMAP=y CONFIG_SYSCON=y +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_MMC_DW=y CONFIG_MMC_DW_ROCKCHIP=y diff --git a/configs/tinker-rk3288_defconfig b/configs/tinker-rk3288_defconfig index 00e2d81..e7eba10 100644 --- a/configs/tinker-rk3288_defconfig +++ b/configs/tinker-rk3288_defconfig @@ -41,6 +41,8 @@ CONFIG_SPL_REGMAP=y CONFIG_SYSCON=y CONFIG_SPL_SYSCON=y # CONFIG_SPL_SIMPLE_BUS is not set +CONFIG_ADC=y +CONFIG_SARADC_ROCKCHIP=y CONFIG_CLK=y CONFIG_SPL_CLK=y CONFIG_ROCKCHIP_GPIO=y -- 2.7.4

On 13 Sep 2017, at 12:09, David Wu david.wu@rock-chips.com wrote:

Commit message?

Signed-off-by: David Wu david.wu@rock-chips.com

Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

See above and below for requested changes.

---

drivers/clk/rockchip/clk_rk3288.c | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+)

diff --git a/drivers/clk/rockchip/clk_rk3288.c b/drivers/clk/rockchip/clk_rk3288.c index 478195b..29652b0 100644 --- a/drivers/clk/rockchip/clk_rk3288.c +++ b/drivers/clk/rockchip/clk_rk3288.c @@ -111,6 +111,15 @@ enum { PERI_ACLK_DIV_SHIFT = 0, PERI_ACLK_DIV_MASK = 0x1f << PERI_ACLK_DIV_SHIFT,

• /*

• * CLKSEL24
  

• * saradc_div_con:
  

• * clk_saradc=24MHz/(saradc_div_con+1)
  

• */
  

• CLK_SARADC_DIV_CON_SHIFT = 8,
• CLK_SARADC_DIV_CON_MASK = 0xff << CLK_SARADC_DIV_CON_SHIFT,
• CLK_SARADC_DIV_CON_WIDTH = 8,
• SOCSTS_DPLL_LOCK = 1 << 5, SOCSTS_APLL_LOCK = 1 << 6, SOCSTS_CPLL_LOCK = 1 << 7,

@@ -131,6 +140,11 @@ static const struct pll_div apll_init_cfg = PLL_DIVISORS(APLL_HZ, 1, 1); static const struct pll_div gpll_init_cfg = PLL_DIVISORS(GPLL_HZ, 2, 2); static const struct pll_div cpll_init_cfg = PLL_DIVISORS(CPLL_HZ, 1, 2);

+static inline u32 extract_bits(u32 val, unsigned width, unsigned shift) +{

• return (val >> shift) & ((1 << width) - 1);

+}

Same comments apply as for patch 3/8.

static int rkclk_set_pll(struct rk3288_cru *cru, enum rk_clk_id clk_id, const struct pll_div *div) { @@ -634,6 +648,31 @@ static ulong rockchip_spi_set_clk(struct rk3288_cru *cru, uint gclk_rate, return rockchip_spi_get_clk(cru, gclk_rate, periph); }

+static ulong rockchip_saradc_get_clk(struct rk3288_cru *cru) +{

• u32 div, val;
• val = readl(&cru->cru_clksel_con[24]);
• div = extract_bits(val, CLK_SARADC_DIV_CON_WIDTH,

• 	   CLK_SARADC_DIV_CON_SHIFT);
  

• return DIV_TO_RATE(OSC_HZ, div);

+}

+static ulong rockchip_saradc_set_clk(struct rk3288_cru *cru, uint hz) +{

• int src_clk_div;
• src_clk_div = DIV_ROUND_UP(OSC_HZ, hz) - 1;
• assert(src_clk_div < 128);
• rk_clrsetreg(&cru->cru_clksel_con[24],

•      CLK_SARADC_DIV_CON_MASK,
  

•      src_clk_div << CLK_SARADC_DIV_CON_SHIFT);
  

• return rockchip_saradc_get_clk(cru);

+}

static ulong rk3288_clk_get_rate(struct clk *clk) { struct rk3288_clk_priv *priv = dev_get_priv(clk->dev); @@ -666,6 +705,9 @@ static ulong rk3288_clk_get_rate(struct clk *clk) return gclk_rate; case PCLK_PWM: return PD_BUS_PCLK_HZ;

• case SCLK_SARADC:

• new_rate = rockchip_saradc_get_clk(priv->cru);
  

• break;
  

  default: return -ENOENT; }

@@ -756,6 +798,9 @@ static ulong rk3288_clk_set_rate(struct clk *clk, ulong rate) new_rate = rate; break; #endif

• case SCLK_SARADC:

• new_rate = rockchip_saradc_set_clk(priv->cru, rate);
  

• break;
  

  default: return -ENOENT; }

-- 2.7.4

On Wed, 13 Sep 2017, David Wu wrote:

The clk_saradc is dividing from the 24M, clk_saradc=24MHz/(saradc_div_con+1). Saradc integer divider control register is 10-bits width.

Signed-off-by: David Wu david.wu@rock-chips.com

Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com

See below for comments.

---

drivers/clk/rockchip/clk_rk3328.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)

diff --git a/drivers/clk/rockchip/clk_rk3328.c b/drivers/clk/rockchip/clk_rk3328.c index c3a6650..e1ae7b2 100644 --- a/drivers/clk/rockchip/clk_rk3328.c +++ b/drivers/clk/rockchip/clk_rk3328.c @@ -115,6 +115,7 @@ enum { /* CLKSEL_CON23 */ CLK_SARADC_DIV_CON_SHIFT = 0, CLK_SARADC_DIV_CON_MASK = 0x3ff << CLK_SARADC_DIV_CON_SHIFT,

Can we use GENMASK here?

• CLK_SARADC_DIV_CON_WIDTH = 10,

  /* CLKSEL_CON24 */ CLK_PWM_PLL_SEL_CPLL = 0,

@@ -180,6 +181,11 @@ enum { #define PLL_DIV_MIN 16 #define PLL_DIV_MAX 3200

+static inline u32 extract_bits(u32 val, unsigned width, unsigned shift) +{

• return (val >> shift) & ((1 << width) - 1);

Can we use the functions from bitfield.h, please?

+}

/*

• How to calculate the PLL(from TRM V0.3 Part 1 Page 63):
• Formulas also embedded within the Fractional PLL Verilog model:

@@ -478,6 +484,31 @@ static ulong rk3328_pwm_set_clk(struct rk3328_cru *cru, uint hz) return DIV_TO_RATE(GPLL_HZ, div); }

+static ulong rk3328_saradc_get_clk(struct rk3328_cru *cru) +{

• u32 div, val;
• val = readl(&cru->clksel_con[23]);
• div = extract_bits(val, CLK_SARADC_DIV_CON_WIDTH,

• 	   CLK_SARADC_DIV_CON_SHIFT);
  

• return DIV_TO_RATE(OSC_HZ, div);

+}

+static ulong rk3328_saradc_set_clk(struct rk3328_cru *cru, uint hz) +{

• int src_clk_div;
• src_clk_div = DIV_ROUND_UP(OSC_HZ, hz) - 1;
• assert(src_clk_div < 128);
• rk_clrsetreg(&cru->clksel_con[23],

•      CLK_SARADC_DIV_CON_MASK,
  

•      src_clk_div << CLK_SARADC_DIV_CON_SHIFT);
  

• return rk3328_saradc_get_clk(cru);

+}

static ulong rk3328_clk_get_rate(struct clk *clk) { struct rk3328_clk_priv *priv = dev_get_priv(clk->dev); @@ -501,6 +532,9 @@ static ulong rk3328_clk_get_rate(struct clk *clk) case SCLK_PWM: rate = rk3328_pwm_get_clk(priv->cru); break;

• case SCLK_SARADC:

• rate = rk3328_saradc_get_clk(priv->cru);
  

• break;
  

  default: return -ENOENT; }

@@ -531,6 +565,9 @@ static ulong rk3328_clk_set_rate(struct clk *clk, ulong rate) case SCLK_PWM: ret = rk3328_pwm_set_clk(priv->cru, rate); break;

• case SCLK_SARADC:

• ret = rk3328_saradc_set_clk(priv->cru, rate);
  

• break;
  

  default: return -ENOENT; }

On 13 Sep 2017, at 22:41, Philipp Tomsich philipp.tomsich@theobroma-systems.com wrote:

On Wed, 13 Sep 2017, David Wu wrote:

...

The clk_saradc is dividing from the 24M, clk_saradc=24MHz/(saradc_div_con+1). Saradc integer divider control register is 8-bits width.

Signed-off-by: David Wu <david.wu@rock-chips.com mailto:david.wu@rock-chips.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com mailto:philipp.tomsich@theobroma-systems.com>

See below for comments.

...

---

arch/arm/include/asm/arch-rockchip/cru_rk3368.h | 5 ++++ drivers/clk/rockchip/clk_rk3368.c | 32 +++++++++++++++++++++++++ 2 files changed, 37 insertions(+)

diff --git a/arch/arm/include/asm/arch-rockchip/cru_rk3368.h b/arch/arm/include/asm/arch-rockchip/cru_rk3368.h index 2b1197f..31f7685 100644 --- a/arch/arm/include/asm/arch-rockchip/cru_rk3368.h +++ b/arch/arm/include/asm/arch-rockchip/cru_rk3368.h @@ -89,6 +89,11 @@ enum { MCU_CLK_DIV_SHIFT = 0, MCU_CLK_DIV_MASK = GENMASK(4, 0),

• /* CLKSEL_CON25 */
• CLK_SARADC_DIV_CON_SHIFT = 8,
• CLK_SARADC_DIV_CON_MASK = 0xff << CLK_SARADC_DIV_CON_SHIFT,

Please use GENMASK.

...

• CLK_SARADC_DIV_CON_WIDTH = 8,
• /* CLKSEL43_CON */ GMAC_MUX_SEL_EXTCLK = BIT(8),

diff --git a/drivers/clk/rockchip/clk_rk3368.c b/drivers/clk/rockchip/clk_rk3368.c index 2be1f57..2eedf77 100644 --- a/drivers/clk/rockchip/clk_rk3368.c +++ b/drivers/clk/rockchip/clk_rk3368.c @@ -12,6 +12,7 @@ #include <errno.h> #include <mapmem.h> #include <syscon.h> +#include <bitfield.h> #include <asm/arch/clock.h> #include <asm/arch/cru_rk3368.h> #include <asm/arch/hardware.h> @@ -397,6 +398,31 @@ static ulong rk3368_spi_set_clk(struct rk3368_cru *cru, ulong clk_id, uint hz) return rk3368_spi_get_clk(cru, clk_id); }

+static ulong rk3368_saradc_get_clk(struct rk3368_cru *cru) +{

• u32 div, val;
• val = readl(&cru->clksel_con[25]);
• div = bitfield_extract(val, CLK_SARADC_DIV_CON_SHIFT,

• 	       CLK_SARADC_DIV_CON_WIDTH);
  

Please reuse the functions from bitfield.h.

It’s apparently too late to do code reviews: please ignore this comment.

...

• return DIV_TO_RATE(OSC_HZ, div);

+}

+static ulong rk3368_saradc_set_clk(struct rk3368_cru *cru, uint hz) +{

• int src_clk_div;
• src_clk_div = DIV_ROUND_UP(OSC_HZ, hz) - 1;
• assert(src_clk_div < 128);
• rk_clrsetreg(&cru->clksel_con[25],

•      CLK_SARADC_DIV_CON_MASK,
  

•      src_clk_div << CLK_SARADC_DIV_CON_SHIFT);
  

• return rk3368_saradc_get_clk(cru);

+}

static ulong rk3368_clk_get_rate(struct clk *clk) { struct rk3368_clk_priv *priv = dev_get_priv(clk->dev); @@ -419,6 +445,9 @@ static ulong rk3368_clk_get_rate(struct clk *clk) rate = rk3368_mmc_get_clk(priv->cru, clk->id); break; #endif

• case SCLK_SARADC:

• rate = rk3368_saradc_get_clk(priv->cru);
  

• break;
  

  default: return -ENOENT; }

@@ -453,6 +482,9 @@ static ulong rk3368_clk_set_rate(struct clk *clk, ulong rate) ret = rk3368_gmac_set_clk(priv->cru, clk->id, rate); break; #endif

• case SCLK_SARADC:

• ret =  rk3368_saradc_set_clk(priv->cru, rate);
  

• break;
  

  default: return -ENOENT; }