Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[MyungJoo Ham]

PiArc3RhdGljIHVuc2lnbmVkIGxvbmcgcmszMzk5X2RtY2Nsa19yZWNhbGNfcmF0ZShzdHJ1Y3Qg
Y2xrX2h3ICpodywNCj4gKwkJCQkJICAgICAgIHVuc2lnbmVkIGxvbmcgcGFyZW50X3JhdGUpDQo+
ICt7DQo+ICsJc3RydWN0IHJrMzM5OV9kbWNjbGsgKmRtYyA9IHRvX3JrMzM5OV9kbWNjbGsoJmh3
KTsNCj4gKwl1MzIgdmFsOw0KPiArDQo+ICsJLyoNCj4gKwkgKiBHZXQgcGFyZW50IHJhdGUgc2lu
Y2UgaXQgY2hhbmdlZCBpbiB0aGlzIGNsa3Mgc2V0X3JhdGUgb3AuIFRoZSBwYXJlbnQNCj4gKwkg
KiByYXRlIHBhc3NlZCBpbnRvIHRoaXMgZnVuY3Rpb24gaXMgY2FjaGVkIGJlZm9yZSBzZXRfcmF0
ZSBpcyBjYWxsZWQgaW4NCj4gKwkgKiB0aGUgY29tbW9uIGNsayBjb2RlLCBzbyB3ZSBoYXZlIHRv
IGdldCBpdCBoZXJlLg0KPiArCSAqLw0KPiArCXBhcmVudF9yYXRlID0gY2xrX2dldF9yYXRlKGNs
a19nZXRfcGFyZW50KGh3LT5jbGspKTsNCj4gKw0KPiArCXZhbCA9IHJlYWRsKGRtYy0+Y3J1ICsg
Q1JVX0NMS1NFTDZfQ09OKTsNCj4gKwl2YWwgPSAodmFsID4+IENMS19ERFJDX0RJVl9DT05fU0hJ
RlQpICYgQ0xLX0REUkNfRElWX0NPTl9NQVNLOw0KPiArDQo+ICsJcmV0dXJuIHBhcmVudF9yYXRl
IC8gKHZhbCArIDEpOw0KPiArfQ0KPiArDQo+ICsvKg0KPiArICogVE9ETzogc2V0IGRkciBmcmVx
dWNuZXkgaW4gZGNmIHdoaWNoIHJ1biBpbiBBVEYNCj4gKyAqLw0KPiArc3RhdGljIGludCByazMz
OTlfZG1jY2xrX3NldF9yYXRlKHN0cnVjdCBjbGtfaHcgKmh3LCB1bnNpZ25lZCBsb25nIHJhdGUs
DQo+ICsJCQkJICB1bnNpZ25lZCBsb25nIHBhcmVudF9yYXRlKQ0KPiArew0KPiArCXJldHVybiAw
Ow0KPiArfQ0KDQpJcyBpdCBjb3JyZWN0IHRoYXQgeW91IGRpZG4ndCBmaWxsIHRoaXMgdXAgYmVj
YXVzZSB5b3VyDQpUcnVzdHpvbmUgZHJpdmVyIChTTUMpIGlzIG5vdCByZWFkeSB5ZXQ/DQoNClRo
ZW4sIHdoeSBkb24ndCB5b3UgZmlsbCB0aGF0IGZ1bmN0aW9uIGFzc3VtaW5nIHRoYXQgVHJ1c3Ra
b25lIGlzIG5vdCBhY3RpdmF0ZWQNCmFuZCBhZGQgU01DIGNhbGwgZnVuY3Rpb25zIHdpdGggaWYg
b3IgI2lmIGFmdGVyIGl0cyBUcnVzdFpvbmUgZHJpdmVyIGlzIHJlYWR5Pw0KDQpPciBkb2VzIHlv
dXIgU29DIG1hbmRhdGUgdGhlIHVzYWdlIG90IFRydXN0Wm9uZSwgcmVzdHJpY3RpbmcgdGhlIHVz
YWdlDQpvZiBDUlVfQ0xLU0VMNl9DT04gd3JpdGU/IChJIGRvbid0IHNlZSB3aHkgU29DIHZlbmRv
cnMgd2lsbCBkbyB0aGlzLi4pDQoNCg0KSSdsbCBiZSByZWFkeSB0byBtZXJnZSB0aGUgUkszMzk5
IGRldmZyZXEgZHJpdmVyIGlmIHlvdQ0KZmlsbCB0aGlzIHVwIChhc3N1bWluZyB0aGF0IFRaIGlz
IG5vdCBlbmFibGVkKSBvcg0KYWRkIFRaIGRyaXZlciBhbmQgU01DIGNhbGxzLg0KDQoNCkNoZWVy
cywNCk15dW5nSm9vDQpwcy4gYWNjb3JkaW5nIHRvIHJrMzM5X2RtY2Nsa19yZWNhbGNfcmF0ZSgp
LCBmaWxsaW5nIHJrMzM5X2RtY2Nsa19zZXRfcmF0ZQ0KYXNzdW1pbmcgdGhhdCBUWiBpcyBub3Qg
ZW5hYmxlZCBzZWVtcyB0cml2aWFsLg==

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[MyungJoo Ham]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1675 bytes --]

> +static unsigned long rk3399_dmcclk_recalc_rate(struct clk_hw *hw,
> +					       unsigned long parent_rate)
> +{
> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
> +	u32 val;
> +
> +	/*
> +	 * Get parent rate since it changed in this clks set_rate op. The parent
> +	 * rate passed into this function is cached before set_rate is called in
> +	 * the common clk code, so we have to get it here.
> +	 */
> +	parent_rate = clk_get_rate(clk_get_parent(hw->clk));
> +
> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
> +	val = (val >> CLK_DDRC_DIV_CON_SHIFT) & CLK_DDRC_DIV_CON_MASK;
> +
> +	return parent_rate / (val + 1);
> +}
> +
> +/*
> + * TODO: set ddr frequcney in dcf which run in ATF
> + */
> +static int rk3399_dmcclk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long parent_rate)
> +{
> +	return 0;
> +}

Is it correct that you didn't fill this up because your
Trustzone driver (SMC) is not ready yet?

Then, why don't you fill that function assuming that TrustZone is not activated
and add SMC call functions with if or #if after its TrustZone driver is ready?

Or does your SoC mandate the usage ot TrustZone, restricting the usage
of CRU_CLKSEL6_CON write? (I don't see why SoC vendors will do this..)


I'll be ready to merge the RK3399 devfreq driver if you
fill this up (assuming that TZ is not enabled) or
add TZ driver and SMC calls.


Cheers,
MyungJoo
ps. according to rk339_dmcclk_recalc_rate(), filling rk339_dmcclk_set_rate
assuming that TZ is not enabled seems trivial.ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[MyungJoo Ham]

> +static unsigned long rk3399_dmcclk_recalc_rate(struct clk_hw *hw,
> +					       unsigned long parent_rate)
> +{
> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
> +	u32 val;
> +
> +	/*
> +	 * Get parent rate since it changed in this clks set_rate op. The parent
> +	 * rate passed into this function is cached before set_rate is called in
> +	 * the common clk code, so we have to get it here.
> +	 */
> +	parent_rate = clk_get_rate(clk_get_parent(hw->clk));
> +
> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
> +	val = (val >> CLK_DDRC_DIV_CON_SHIFT) & CLK_DDRC_DIV_CON_MASK;
> +
> +	return parent_rate / (val + 1);
> +}
> +
> +/*
> + * TODO: set ddr frequcney in dcf which run in ATF
> + */
> +static int rk3399_dmcclk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long parent_rate)
> +{
> +	return 0;
> +}

Is it correct that you didn't fill this up because your
Trustzone driver (SMC) is not ready yet?

Then, why don't you fill that function assuming that TrustZone is not activated
and add SMC call functions with if or #if after its TrustZone driver is ready?

Or does your SoC mandate the usage ot TrustZone, restricting the usage
of CRU_CLKSEL6_CON write? (I don't see why SoC vendors will do this..)


I'll be ready to merge the RK3399 devfreq driver if you
fill this up (assuming that TZ is not enabled) or
add TZ driver and SMC calls.


Cheers,
MyungJoo
ps. according to rk339_dmcclk_recalc_rate(), filling rk339_dmcclk_set_rate
assuming that TZ is not enabled seems trivial.

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[hl]

Hi MyungJoo,

On 23/11/15 16:09, MyungJoo Ham wrote:
>> +static unsigned long rk3399_dmcclk_recalc_rate(struct clk_hw *hw,
>> +					       unsigned long parent_rate)
>> +{
>> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
>> +	u32 val;
>> +
>> +	/*
>> +	 * Get parent rate since it changed in this clks set_rate op. The parent
>> +	 * rate passed into this function is cached before set_rate is called in
>> +	 * the common clk code, so we have to get it here.
>> +	 */
>> +	parent_rate = clk_get_rate(clk_get_parent(hw->clk));
>> +
>> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
>> +	val = (val >> CLK_DDRC_DIV_CON_SHIFT) & CLK_DDRC_DIV_CON_MASK;
>> +
>> +	return parent_rate / (val + 1);
>> +}
>> +
>> +/*
>> + * TODO: set ddr frequcney in dcf which run in ATF
>> + */
>> +static int rk3399_dmcclk_set_rate(struct clk_hw *hw, unsigned long rate,
>> +				  unsigned long parent_rate)
>> +{
>> +	return 0;
>> +}
> Is it correct that you didn't fill this up because your
> Trustzone driver (SMC) is not ready yet?
     Yep, the SMC is not ready yet.
>
> Then, why don't you fill that function assuming that TrustZone is not activated
> and add SMC call functions with if or #if after its TrustZone driver is ready?
>
> Or does your SoC mandate the usage ot TrustZone, restricting the usage
> of CRU_CLKSEL6_CON write? (I don't see why SoC vendors will do this..)
>
>
> I'll be ready to merge the RK3399 devfreq driver if you
> fill this up (assuming that TZ is not enabled) or
> add TZ driver and SMC calls.
     Thank you for your reply,  it is good idea use if or #if to 
distinguish the TrustZone whether ready,
     i will handle it in next version. I may follow Heiko advice to do 
some modify in dmc clock and rk3399 devfreq driver,
     I will upload new version when it's ready.
>
> Cheers,
> MyungJoo
> ps. according to rk339_dmcclk_recalc_rate(), filling rk339_dmcclk_set_rate
> assuming that TZ is not enabled seems trivial.

-- 
Lin Huang

[PATCH 0/2] Bring up rk3399 ddr frequency scaling

[Lin Huang]

these patchset bring up rk3399 ddr frequency scaling flow,
use devfreq framework and simple_ondemand policy. Ddr set
rate function will implement in dcf controller which run in
the ATF, and rk3399 ATF not ready now, so we need finish it
when rk3399 ATF ready.

Lin Huang (2):
  clk: rockchip: dmc: support rk3399 dmc clock driver
  devfreq: rockchip: support rk3399 dmc devfreq

 drivers/clk/rockchip/Makefile         |   1 +
 drivers/clk/rockchip/clk-rk3399-dmc.c | 196 ++++++++++++++++++++
 drivers/devfreq/Kconfig               |   1 +
 drivers/devfreq/Makefile              |   1 +
 drivers/devfreq/rockchip/Kconfig      |   8 +
 drivers/devfreq/rockchip/Makefile     |   1 +
 drivers/devfreq/rockchip/rk3399_dmc.c | 327 ++++++++++++++++++++++++++++++++++
 include/soc/rockchip/rk3399-dmc-clk.h |  36 ++++
 8 files changed, 571 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-rk3399-dmc.c
 create mode 100644 drivers/devfreq/rockchip/Kconfig
 create mode 100644 drivers/devfreq/rockchip/Makefile
 create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
 create mode 100644 include/soc/rockchip/rk3399-dmc-clk.h

-- 
1.9.1

[PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[Lin Huang]

support rk3399 dmc clock driver. Note, ddr set rate function will
use dcf controller which run in ATF, it need to fishish it when rk3399
arm trust firmware ready.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 drivers/clk/rockchip/Makefile         |   1 +
 drivers/clk/rockchip/clk-rk3399-dmc.c | 196 ++++++++++++++++++++++++++++++++++
 include/soc/rockchip/rk3399-dmc-clk.h |  36 +++++++
 3 files changed, 233 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-rk3399-dmc.c
 create mode 100644 include/soc/rockchip/rk3399-dmc-clk.h

diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
index b27edd6..98bd955 100644
--- a/drivers/clk/rockchip/Makefile
+++ b/drivers/clk/rockchip/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_RESET_CONTROLLER)	+= softrst.o
 obj-y	+= clk-rk3188.o
 obj-y	+= clk-rk3288.o
 obj-y	+= clk-rk3368.o
+obj-y	+= clk-rk3399-dmc.o
diff --git a/drivers/clk/rockchip/clk-rk3399-dmc.c b/drivers/clk/rockchip/clk-rk3399-dmc.c
new file mode 100644
index 0000000..03cc044
--- /dev/null
+++ b/drivers/clk/rockchip/clk-rk3399-dmc.c
@@ -0,0 +1,196 @@
+/*
+ * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/slab.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <soc/rockchip/rk3399-dmc-clk.h>
+
+#define to_rk3399_dmcclk(obj)	container_of(obj, struct rk3399_dmcclk, hw)
+
+/* CRU_CLKSEL6_CON*/
+#define CRU_CLKSEL6_CON		0x118
+#define CLK_DDRC_PLL_SEL_SHIFT	0x4
+#define CLK_DDRC_PLL_SEL_MASK	0x3
+#define CLK_DDRC_DIV_CON_SHIFT	0
+#define CLK_DDRC_DIV_CON_MASK	0x07
+
+static unsigned long rk3399_dmcclk_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
+	u32 val;
+
+	/*
+	 * Get parent rate since it changed in this clks set_rate op. The parent
+	 * rate passed into this function is cached before set_rate is called in
+	 * the common clk code, so we have to get it here.
+	 */
+	parent_rate = clk_get_rate(clk_get_parent(hw->clk));
+
+	val = readl(dmc->cru + CRU_CLKSEL6_CON);
+	val = (val >> CLK_DDRC_DIV_CON_SHIFT) & CLK_DDRC_DIV_CON_MASK;
+
+	return parent_rate / (val + 1);
+}
+
+/*
+ * TODO: set ddr frequcney in dcf which run in ATF
+ */
+static int rk3399_dmcclk_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long parent_rate)
+{
+	return 0;
+}
+
+static u8 rk3399_dmcclk_get_parent(struct clk_hw *hw)
+{
+	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
+	u32 val;
+
+	val = readl(dmc->cru + CRU_CLKSEL6_CON);
+
+	return (val >> CLK_DDRC_PLL_SEL_SHIFT) &
+		CLK_DDRC_PLL_SEL_MASK;
+}
+
+static const struct clk_ops rk3399_dmcclk_ops = {
+	.recalc_rate = rk3399_dmcclk_recalc_rate,
+	.set_rate = rk3399_dmcclk_set_rate,
+	.get_parent = rk3399_dmcclk_get_parent,
+};
+
+static const char *parent_clk_names[] = {
+	"pll_dpll",
+	"pll_gpll",
+	"pll_alpll",
+	"pll_abpll",
+};
+
+static int rk3399_register_dmcclk(struct rk3399_dmcclk *dmc)
+{
+	struct clk_init_data init;
+	struct clk *clk;
+
+	init.name = "dmc_clk";
+	init.parent_names = parent_clk_names;
+	init.num_parents = ARRAY_SIZE(parent_clk_names);
+	init.ops = &rk3399_dmcclk_ops;
+	init.flags = 0;
+	dmc->hw->init = &init;
+
+	clk = devm_clk_register(dmc->dev, dmc->hw);
+	if (IS_ERR(clk)) {
+		dev_err(dmc->dev, "could not register cpuclk dmc_clk\n");
+		return PTR_ERR(clk);
+	}
+	clk_register_clkdev(clk, "dmc_clk", NULL);
+	of_clk_add_provider(dmc->dev->of_node, of_clk_src_simple_get, clk);
+
+	return 0;
+}
+
+static int rk3399_dmcclk_probe(struct platform_device *pdev)
+{
+	struct rk3399_dmcclk *dmc;
+	struct resource *res;
+	struct device_node *node;
+	int ret;
+
+	dmc = devm_kzalloc(&pdev->dev, sizeof(*dmc), GFP_KERNEL);
+	if (!dmc)
+		return -ENOMEM;
+
+	dmc->hw = devm_kzalloc(&pdev->dev, sizeof(*dmc->hw), GFP_KERNEL);
+	if (!dmc->hw)
+		return -ENOMEM;
+
+	dmc->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	dmc->ctrl_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dmc->ctrl_regs))
+		return PTR_ERR(dmc->ctrl_regs);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	dmc->dfi_regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dmc->dfi_regs))
+		return PTR_ERR(dmc->dfi_regs);
+
+	node = of_parse_phandle(dmc->dev->of_node, "rockchip,cru", 0);
+	if (!node)
+		return -ENODEV;
+
+	ret = of_address_to_resource(node, 0, res);
+	if (ret)
+		return ret;
+
+	dmc->cru = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dmc->cru))
+		return PTR_ERR(dmc->cru);
+
+	/* register dpllddr clock */
+	ret = rk3399_register_dmcclk(dmc);
+	if (ret) {
+		dev_err(dmc->dev, "failed to register clk dmc_clk %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, dmc);
+	platform_device_register_data(dmc->dev, "rk3399-dmc-freq",
+				      PLATFORM_DEVID_AUTO, NULL, 0);
+
+	return 0;
+}
+
+static const struct of_device_id rk3399_dmcclk_of_match[] = {
+	{ .compatible = "rockchip,rk3399-dmc", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, rk3399_dmcclk_of_match);
+
+
+static struct platform_driver rk3399_dmcclk_driver = {
+	.probe = rk3399_dmcclk_probe,
+	.driver = {
+		.name = "rk3399-dmc",
+		.of_match_table = rk3399_dmcclk_of_match,
+		.suppress_bind_attrs = true,
+	},
+};
+
+static int __init rk3399_dmcclk_modinit(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&rk3399_dmcclk_driver);
+	if (ret < 0)
+		pr_err("Failed to register platform driver %s\n",
+				rk3399_dmcclk_driver.driver.name);
+
+	return ret;
+}
+
+module_init(rk3399_dmcclk_modinit);
+
+MODULE_DESCRIPTION("rockchip rk3399 DMC CLK driver");
+MODULE_LICENSE("GPL v2");
+
diff --git a/include/soc/rockchip/rk3399-dmc-clk.h b/include/soc/rockchip/rk3399-dmc-clk.h
new file mode 100644
index 0000000..b53fc23
--- /dev/null
+++ b/include/soc/rockchip/rk3399-dmc-clk.h
@@ -0,0 +1,36 @@
+/*
+* Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
+*
+* 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.
+*/
+
+#ifndef _RK3399_DMC_CLK_H
+#define _RK3399_DMC_CLK_H
+
+enum dram_type_tag {
+	DDR3 = 6,
+	LPDDR3 = 7,
+	LPDDR4 = 0x0b,
+};
+
+/* DENALI_CTL_00 */
+#define DENALI_CTL_00		0x00
+#define DRAM_CLASS_MASK		0x0f
+#define DRAM_CLASS_SHIFT	0x8
+
+struct rk3399_dmcclk {
+	struct device *dev;
+	struct clk_hw *hw;
+	u32 cur_freq;
+	u32 target_freq;
+	u32 ddr_type;
+	void __iomem *ctrl_regs;
+	void __iomem *dfi_regs;
+	void __iomem *cru;
+};
+
+#endif
+
-- 
1.9.1

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[Heiko Stuebner]

Hi Lin,

Am Donnerstag, 19. November 2015, 18:21:10 schrieb Lin Huang:
> support rk3399 dmc clock driver. Note, ddr set rate function will
> use dcf controller which run in ATF, it need to fishish it when rk3399
> arm trust firmware ready.

this unfinalized state is slightly unfortunate and I think this code will
need to wait until CRU and DDRC specs are available.

Because this is clearly part of the CRU (labeled CRU_CLKSEL6_CON etc)
so shouldn't be a separate driver at all and also what your driver currently
only does can still simply be described in the regular scheme as part of
a full clock driver like

PNAME(mux_ddrc_p) = { "pll_dpll", "pll_gpll", "pll_alpll", "pll_abpll" };
COMPOSITE_NOGATE(0, "ddrc", mux_ddrc_p, 0,
					RK3399_CLKSEL_CON(6), 4, 2, MFLAGS, 0, 3,
					DFLAGS | CLK_DIVIDER_POWER_OF_TWO),

So the code needs to actually demonstrate why a separate clock type is
really necessary.

I do understand that we will probably need a special way to talk to this
dcf controller but seeing how this interaction will work is really a
prequisite to finding a correct solution.


Heiko

> ---
>  drivers/clk/rockchip/Makefile         |   1 +
>  drivers/clk/rockchip/clk-rk3399-dmc.c | 196 ++++++++++++++++++++++++++++++++++
>  include/soc/rockchip/rk3399-dmc-clk.h |  36 +++++++
>  3 files changed, 233 insertions(+)
>  create mode 100644 drivers/clk/rockchip/clk-rk3399-dmc.c
>  create mode 100644 include/soc/rockchip/rk3399-dmc-clk.h
> 
> diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
> index b27edd6..98bd955 100644
> --- a/drivers/clk/rockchip/Makefile
> +++ b/drivers/clk/rockchip/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_RESET_CONTROLLER)	+= softrst.o
>  obj-y	+= clk-rk3188.o
>  obj-y	+= clk-rk3288.o
>  obj-y	+= clk-rk3368.o
> +obj-y	+= clk-rk3399-dmc.o
> diff --git a/drivers/clk/rockchip/clk-rk3399-dmc.c b/drivers/clk/rockchip/clk-rk3399-dmc.c
> new file mode 100644
> index 0000000..03cc044
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-rk3399-dmc.c
> @@ -0,0 +1,196 @@
> +/*
> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/slab.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <soc/rockchip/rk3399-dmc-clk.h>
> +
> +#define to_rk3399_dmcclk(obj)	container_of(obj, struct rk3399_dmcclk, hw)
> +
> +/* CRU_CLKSEL6_CON*/
> +#define CRU_CLKSEL6_CON		0x118
> +#define CLK_DDRC_PLL_SEL_SHIFT	0x4
> +#define CLK_DDRC_PLL_SEL_MASK	0x3
> +#define CLK_DDRC_DIV_CON_SHIFT	0
> +#define CLK_DDRC_DIV_CON_MASK	0x07
> +
> +static unsigned long rk3399_dmcclk_recalc_rate(struct clk_hw *hw,
> +					       unsigned long parent_rate)
> +{
> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
> +	u32 val;
> +
> +	/*
> +	 * Get parent rate since it changed in this clks set_rate op. The parent
> +	 * rate passed into this function is cached before set_rate is called in
> +	 * the common clk code, so we have to get it here.
> +	 */
> +	parent_rate = clk_get_rate(clk_get_parent(hw->clk));
> +
> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
> +	val = (val >> CLK_DDRC_DIV_CON_SHIFT) & CLK_DDRC_DIV_CON_MASK;
> +
> +	return parent_rate / (val + 1);
> +}
> +
> +/*
> + * TODO: set ddr frequcney in dcf which run in ATF
> + */
> +static int rk3399_dmcclk_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long parent_rate)
> +{
> +	return 0;
> +}
> +
> +static u8 rk3399_dmcclk_get_parent(struct clk_hw *hw)
> +{
> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
> +	u32 val;
> +
> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
> +
> +	return (val >> CLK_DDRC_PLL_SEL_SHIFT) &
> +		CLK_DDRC_PLL_SEL_MASK;
> +}
> +
> +static const struct clk_ops rk3399_dmcclk_ops = {
> +	.recalc_rate = rk3399_dmcclk_recalc_rate,
> +	.set_rate = rk3399_dmcclk_set_rate,
> +	.get_parent = rk3399_dmcclk_get_parent,
> +};
> +
> +static const char *parent_clk_names[] = {
> +	"pll_dpll",
> +	"pll_gpll",
> +	"pll_alpll",
> +	"pll_abpll",
> +};
> +
> +static int rk3399_register_dmcclk(struct rk3399_dmcclk *dmc)
> +{
> +	struct clk_init_data init;
> +	struct clk *clk;
> +
> +	init.name = "dmc_clk";
> +	init.parent_names = parent_clk_names;
> +	init.num_parents = ARRAY_SIZE(parent_clk_names);
> +	init.ops = &rk3399_dmcclk_ops;
> +	init.flags = 0;
> +	dmc->hw->init = &init;
> +
> +	clk = devm_clk_register(dmc->dev, dmc->hw);
> +	if (IS_ERR(clk)) {
> +		dev_err(dmc->dev, "could not register cpuclk dmc_clk\n");
> +		return PTR_ERR(clk);
> +	}
> +	clk_register_clkdev(clk, "dmc_clk", NULL);
> +	of_clk_add_provider(dmc->dev->of_node, of_clk_src_simple_get, clk);
> +
> +	return 0;
> +}
> +
> +static int rk3399_dmcclk_probe(struct platform_device *pdev)
> +{
> +	struct rk3399_dmcclk *dmc;
> +	struct resource *res;
> +	struct device_node *node;
> +	int ret;
> +
> +	dmc = devm_kzalloc(&pdev->dev, sizeof(*dmc), GFP_KERNEL);
> +	if (!dmc)
> +		return -ENOMEM;
> +
> +	dmc->hw = devm_kzalloc(&pdev->dev, sizeof(*dmc->hw), GFP_KERNEL);
> +	if (!dmc->hw)
> +		return -ENOMEM;
> +
> +	dmc->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	dmc->ctrl_regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(dmc->ctrl_regs))
> +		return PTR_ERR(dmc->ctrl_regs);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	dmc->dfi_regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(dmc->dfi_regs))
> +		return PTR_ERR(dmc->dfi_regs);
> +
> +	node = of_parse_phandle(dmc->dev->of_node, "rockchip,cru", 0);
> +	if (!node)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(node, 0, res);
> +	if (ret)
> +		return ret;
> +
> +	dmc->cru = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(dmc->cru))
> +		return PTR_ERR(dmc->cru);
> +
> +	/* register dpllddr clock */
> +	ret = rk3399_register_dmcclk(dmc);
> +	if (ret) {
> +		dev_err(dmc->dev, "failed to register clk dmc_clk %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, dmc);
> +	platform_device_register_data(dmc->dev, "rk3399-dmc-freq",
> +				      PLATFORM_DEVID_AUTO, NULL, 0);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rk3399_dmcclk_of_match[] = {
> +	{ .compatible = "rockchip,rk3399-dmc", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, rk3399_dmcclk_of_match);
> +
> +
> +static struct platform_driver rk3399_dmcclk_driver = {
> +	.probe = rk3399_dmcclk_probe,
> +	.driver = {
> +		.name = "rk3399-dmc",
> +		.of_match_table = rk3399_dmcclk_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +
> +static int __init rk3399_dmcclk_modinit(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&rk3399_dmcclk_driver);
> +	if (ret < 0)
> +		pr_err("Failed to register platform driver %s\n",
> +				rk3399_dmcclk_driver.driver.name);
> +
> +	return ret;
> +}
> +
> +module_init(rk3399_dmcclk_modinit);
> +
> +MODULE_DESCRIPTION("rockchip rk3399 DMC CLK driver");
> +MODULE_LICENSE("GPL v2");
> +
> diff --git a/include/soc/rockchip/rk3399-dmc-clk.h b/include/soc/rockchip/rk3399-dmc-clk.h
> new file mode 100644
> index 0000000..b53fc23
> --- /dev/null
> +++ b/include/soc/rockchip/rk3399-dmc-clk.h
> @@ -0,0 +1,36 @@
> +/*
> +* Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
> +*
> +* 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.
> +*/
> +
> +#ifndef _RK3399_DMC_CLK_H
> +#define _RK3399_DMC_CLK_H
> +
> +enum dram_type_tag {
> +	DDR3 = 6,
> +	LPDDR3 = 7,
> +	LPDDR4 = 0x0b,
> +};
> +
> +/* DENALI_CTL_00 */
> +#define DENALI_CTL_00		0x00
> +#define DRAM_CLASS_MASK		0x0f
> +#define DRAM_CLASS_SHIFT	0x8
> +
> +struct rk3399_dmcclk {
> +	struct device *dev;
> +	struct clk_hw *hw;
> +	u32 cur_freq;
> +	u32 target_freq;
> +	u32 ddr_type;
> +	void __iomem *ctrl_regs;
> +	void __iomem *dfi_regs;
> +	void __iomem *cru;
> +};
> +
> +#endif
> +
> 

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[hl]

Hi Heiko,

On 20/11/15 05:47, Heiko Stuebner wrote:
> Hi Lin,
>
> Am Donnerstag, 19. November 2015, 18:21:10 schrieb Lin Huang:
>> support rk3399 dmc clock driver. Note, ddr set rate function will
>> use dcf controller which run in ATF, it need to fishish it when rk3399
>> arm trust firmware ready.
> this unfinalized state is slightly unfortunate and I think this code will
> need to wait until CRU and DDRC specs are available.
>
> Because this is clearly part of the CRU (labeled CRU_CLKSEL6_CON etc)
> so shouldn't be a separate driver at all and also what your driver currently
> only does can still simply be described in the regular scheme as part of
> a full clock driver like
>
> PNAME(mux_ddrc_p) = { "pll_dpll", "pll_gpll", "pll_alpll", "pll_abpll" };
> COMPOSITE_NOGATE(0, "ddrc", mux_ddrc_p, 0,
> 					RK3399_CLKSEL_CON(6), 4, 2, MFLAGS, 0, 3,
> 					DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
>
> So the code needs to actually demonstrate why a separate clock type is
> really necessary.
>
> I do understand that we will probably need a special way to talk to this
> dcf controller but seeing how this interaction will work is really a
> prequisite to finding a correct solution.
     if we can use common clock driver, i can put the dfi controller as 
a  independent driver
     into devfreq,  this is the best way. But how do we separate the ddr 
clk_set_rate() ,
     so we can manipulate dcf controller(actually, we use SMC handle dcf 
controller in arm trust firmware ),
     i check clock driver code, it seem there is not way to do that for now.
>
> Heiko
>
>> ---
>>   drivers/clk/rockchip/Makefile         |   1 +
>>   drivers/clk/rockchip/clk-rk3399-dmc.c | 196 ++++++++++++++++++++++++++++++++++
>>   include/soc/rockchip/rk3399-dmc-clk.h |  36 +++++++
>>   3 files changed, 233 insertions(+)
>>   create mode 100644 drivers/clk/rockchip/clk-rk3399-dmc.c
>>   create mode 100644 include/soc/rockchip/rk3399-dmc-clk.h
>>
>> diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
>> index b27edd6..98bd955 100644
>> --- a/drivers/clk/rockchip/Makefile
>> +++ b/drivers/clk/rockchip/Makefile
>> @@ -13,3 +13,4 @@ obj-$(CONFIG_RESET_CONTROLLER)	+= softrst.o
>>   obj-y	+= clk-rk3188.o
>>   obj-y	+= clk-rk3288.o
>>   obj-y	+= clk-rk3368.o
>> +obj-y	+= clk-rk3399-dmc.o
>> diff --git a/drivers/clk/rockchip/clk-rk3399-dmc.c b/drivers/clk/rockchip/clk-rk3399-dmc.c
>> new file mode 100644
>> index 0000000..03cc044
>> --- /dev/null
>> +++ b/drivers/clk/rockchip/clk-rk3399-dmc.c
>> @@ -0,0 +1,196 @@
>> +/*
>> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/clkdev.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <soc/rockchip/rk3399-dmc-clk.h>
>> +
>> +#define to_rk3399_dmcclk(obj)	container_of(obj, struct rk3399_dmcclk, hw)
>> +
>> +/* CRU_CLKSEL6_CON*/
>> +#define CRU_CLKSEL6_CON		0x118
>> +#define CLK_DDRC_PLL_SEL_SHIFT	0x4
>> +#define CLK_DDRC_PLL_SEL_MASK	0x3
>> +#define CLK_DDRC_DIV_CON_SHIFT	0
>> +#define CLK_DDRC_DIV_CON_MASK	0x07
>> +
>> +static unsigned long rk3399_dmcclk_recalc_rate(struct clk_hw *hw,
>> +					       unsigned long parent_rate)
>> +{
>> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
>> +	u32 val;
>> +
>> +	/*
>> +	 * Get parent rate since it changed in this clks set_rate op. The parent
>> +	 * rate passed into this function is cached before set_rate is called in
>> +	 * the common clk code, so we have to get it here.
>> +	 */
>> +	parent_rate = clk_get_rate(clk_get_parent(hw->clk));
>> +
>> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
>> +	val = (val >> CLK_DDRC_DIV_CON_SHIFT) & CLK_DDRC_DIV_CON_MASK;
>> +
>> +	return parent_rate / (val + 1);
>> +}
>> +
>> +/*
>> + * TODO: set ddr frequcney in dcf which run in ATF
>> + */
>> +static int rk3399_dmcclk_set_rate(struct clk_hw *hw, unsigned long rate,
>> +				  unsigned long parent_rate)
>> +{
>> +	return 0;
>> +}
>> +
>> +static u8 rk3399_dmcclk_get_parent(struct clk_hw *hw)
>> +{
>> +	struct rk3399_dmcclk *dmc = to_rk3399_dmcclk(&hw);
>> +	u32 val;
>> +
>> +	val = readl(dmc->cru + CRU_CLKSEL6_CON);
>> +
>> +	return (val >> CLK_DDRC_PLL_SEL_SHIFT) &
>> +		CLK_DDRC_PLL_SEL_MASK;
>> +}
>> +
>> +static const struct clk_ops rk3399_dmcclk_ops = {
>> +	.recalc_rate = rk3399_dmcclk_recalc_rate,
>> +	.set_rate = rk3399_dmcclk_set_rate,
>> +	.get_parent = rk3399_dmcclk_get_parent,
>> +};
>> +
>> +static const char *parent_clk_names[] = {
>> +	"pll_dpll",
>> +	"pll_gpll",
>> +	"pll_alpll",
>> +	"pll_abpll",
>> +};
>> +
>> +static int rk3399_register_dmcclk(struct rk3399_dmcclk *dmc)
>> +{
>> +	struct clk_init_data init;
>> +	struct clk *clk;
>> +
>> +	init.name = "dmc_clk";
>> +	init.parent_names = parent_clk_names;
>> +	init.num_parents = ARRAY_SIZE(parent_clk_names);
>> +	init.ops = &rk3399_dmcclk_ops;
>> +	init.flags = 0;
>> +	dmc->hw->init = &init;
>> +
>> +	clk = devm_clk_register(dmc->dev, dmc->hw);
>> +	if (IS_ERR(clk)) {
>> +		dev_err(dmc->dev, "could not register cpuclk dmc_clk\n");
>> +		return PTR_ERR(clk);
>> +	}
>> +	clk_register_clkdev(clk, "dmc_clk", NULL);
>> +	of_clk_add_provider(dmc->dev->of_node, of_clk_src_simple_get, clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk3399_dmcclk_probe(struct platform_device *pdev)
>> +{
>> +	struct rk3399_dmcclk *dmc;
>> +	struct resource *res;
>> +	struct device_node *node;
>> +	int ret;
>> +
>> +	dmc = devm_kzalloc(&pdev->dev, sizeof(*dmc), GFP_KERNEL);
>> +	if (!dmc)
>> +		return -ENOMEM;
>> +
>> +	dmc->hw = devm_kzalloc(&pdev->dev, sizeof(*dmc->hw), GFP_KERNEL);
>> +	if (!dmc->hw)
>> +		return -ENOMEM;
>> +
>> +	dmc->dev = &pdev->dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	dmc->ctrl_regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(dmc->ctrl_regs))
>> +		return PTR_ERR(dmc->ctrl_regs);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +	dmc->dfi_regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(dmc->dfi_regs))
>> +		return PTR_ERR(dmc->dfi_regs);
>> +
>> +	node = of_parse_phandle(dmc->dev->of_node, "rockchip,cru", 0);
>> +	if (!node)
>> +		return -ENODEV;
>> +
>> +	ret = of_address_to_resource(node, 0, res);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dmc->cru = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(dmc->cru))
>> +		return PTR_ERR(dmc->cru);
>> +
>> +	/* register dpllddr clock */
>> +	ret = rk3399_register_dmcclk(dmc);
>> +	if (ret) {
>> +		dev_err(dmc->dev, "failed to register clk dmc_clk %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, dmc);
>> +	platform_device_register_data(dmc->dev, "rk3399-dmc-freq",
>> +				      PLATFORM_DEVID_AUTO, NULL, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id rk3399_dmcclk_of_match[] = {
>> +	{ .compatible = "rockchip,rk3399-dmc", },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, rk3399_dmcclk_of_match);
>> +
>> +
>> +static struct platform_driver rk3399_dmcclk_driver = {
>> +	.probe = rk3399_dmcclk_probe,
>> +	.driver = {
>> +		.name = "rk3399-dmc",
>> +		.of_match_table = rk3399_dmcclk_of_match,
>> +		.suppress_bind_attrs = true,
>> +	},
>> +};
>> +
>> +static int __init rk3399_dmcclk_modinit(void)
>> +{
>> +	int ret;
>> +
>> +	ret = platform_driver_register(&rk3399_dmcclk_driver);
>> +	if (ret < 0)
>> +		pr_err("Failed to register platform driver %s\n",
>> +				rk3399_dmcclk_driver.driver.name);
>> +
>> +	return ret;
>> +}
>> +
>> +module_init(rk3399_dmcclk_modinit);
>> +
>> +MODULE_DESCRIPTION("rockchip rk3399 DMC CLK driver");
>> +MODULE_LICENSE("GPL v2");
>> +
>> diff --git a/include/soc/rockchip/rk3399-dmc-clk.h b/include/soc/rockchip/rk3399-dmc-clk.h
>> new file mode 100644
>> index 0000000..b53fc23
>> --- /dev/null
>> +++ b/include/soc/rockchip/rk3399-dmc-clk.h
>> @@ -0,0 +1,36 @@
>> +/*
>> +* Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
>> +*
>> +* 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.
>> +*/
>> +
>> +#ifndef _RK3399_DMC_CLK_H
>> +#define _RK3399_DMC_CLK_H
>> +
>> +enum dram_type_tag {
>> +	DDR3 = 6,
>> +	LPDDR3 = 7,
>> +	LPDDR4 = 0x0b,
>> +};
>> +
>> +/* DENALI_CTL_00 */
>> +#define DENALI_CTL_00		0x00
>> +#define DRAM_CLASS_MASK		0x0f
>> +#define DRAM_CLASS_SHIFT	0x8
>> +
>> +struct rk3399_dmcclk {
>> +	struct device *dev;
>> +	struct clk_hw *hw;
>> +	u32 cur_freq;
>> +	u32 target_freq;
>> +	u32 ddr_type;
>> +	void __iomem *ctrl_regs;
>> +	void __iomem *dfi_regs;
>> +	void __iomem *cru;
>> +};
>> +
>> +#endif
>> +
>>
>
>
>

-- 
Lin Huang

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[Heiko Stuebner]

Hi Lin,

Am Freitag, 20. November 2015, 09:37:15 schrieb hl:
> On 20/11/15 05:47, Heiko Stuebner wrote:
> > Hi Lin,
> >
> > Am Donnerstag, 19. November 2015, 18:21:10 schrieb Lin Huang:
> >> support rk3399 dmc clock driver. Note, ddr set rate function will
> >> use dcf controller which run in ATF, it need to fishish it when rk3399
> >> arm trust firmware ready.
> > this unfinalized state is slightly unfortunate and I think this code will
> > need to wait until CRU and DDRC specs are available.
> >
> > Because this is clearly part of the CRU (labeled CRU_CLKSEL6_CON etc)
> > so shouldn't be a separate driver at all and also what your driver currently
> > only does can still simply be described in the regular scheme as part of
> > a full clock driver like
> >
> > PNAME(mux_ddrc_p) = { "pll_dpll", "pll_gpll", "pll_alpll", "pll_abpll" };
> > COMPOSITE_NOGATE(0, "ddrc", mux_ddrc_p, 0,
> > 					RK3399_CLKSEL_CON(6), 4, 2, MFLAGS, 0, 3,
> > 					DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
> >
> > So the code needs to actually demonstrate why a separate clock type is
> > really necessary.
> >
> > I do understand that we will probably need a special way to talk to this
> > dcf controller but seeing how this interaction will work is really a
> > prequisite to finding a correct solution.
>
>      if we can use common clock driver, i can put the dfi controller as 
> a  independent driver
>      into devfreq,  this is the best way. But how do we separate the ddr 
> clk_set_rate() ,
>      so we can manipulate dcf controller(actually, we use SMC handle dcf 
> controller in arm trust firmware ),
>      i check clock driver code, it seem there is not way to do that for now.

the core problem I have right now is, that I don't understand how the
interaction with the dfi controller works at all :-) .

One thing that might work, is that your dfi-driver takes the ddrc-clock
from the clock controller and then registers a clock notifier to get
notified before and after the clock-rate changes. But that depends as
stated above on how the dfi-controller needs to be handled.

For example the current armclk-handling uses a clock notifier around the
actual rate change ... so you could use that as inspiration.


Heiko

Re: [PATCH 1/2] clk: rockchip: dmc: support rk3399 dmc clock driver

[hl]

Hi Heiko,

On 22/11/15 02:30, Heiko Stuebner wrote:
> Hi Lin,
>
> Am Freitag, 20. November 2015, 09:37:15 schrieb hl:
>> On 20/11/15 05:47, Heiko Stuebner wrote:
>>> Hi Lin,
>>>
>>> Am Donnerstag, 19. November 2015, 18:21:10 schrieb Lin Huang:
>>>> support rk3399 dmc clock driver. Note, ddr set rate function will
>>>> use dcf controller which run in ATF, it need to fishish it when rk3399
>>>> arm trust firmware ready.
>>> this unfinalized state is slightly unfortunate and I think this code will
>>> need to wait until CRU and DDRC specs are available.
>>>
>>> Because this is clearly part of the CRU (labeled CRU_CLKSEL6_CON etc)
>>> so shouldn't be a separate driver at all and also what your driver currently
>>> only does can still simply be described in the regular scheme as part of
>>> a full clock driver like
>>>
>>> PNAME(mux_ddrc_p) = { "pll_dpll", "pll_gpll", "pll_alpll", "pll_abpll" };
>>> COMPOSITE_NOGATE(0, "ddrc", mux_ddrc_p, 0,
>>> 					RK3399_CLKSEL_CON(6), 4, 2, MFLAGS, 0, 3,
>>> 					DFLAGS | CLK_DIVIDER_POWER_OF_TWO),
>>>
>>> So the code needs to actually demonstrate why a separate clock type is
>>> really necessary.
>>>
>>> I do understand that we will probably need a special way to talk to this
>>> dcf controller but seeing how this interaction will work is really a
>>> prequisite to finding a correct solution.
>>       if we can use common clock driver, i can put the dfi controller as
>> a  independent driver
>>       into devfreq,  this is the best way. But how do we separate the ddr
>> clk_set_rate() ,
>>       so we can manipulate dcf controller(actually, we use SMC handle dcf
>> controller in arm trust firmware ),
>>       i check clock driver code, it seem there is not way to do that for now.
> the core problem I have right now is, that I don't understand how the
> interaction with the dfi controller works at all :-) .
>
> One thing that might work, is that your dfi-driver takes the ddrc-clock
> from the clock controller and then registers a clock notifier to get
> notified before and after the clock-rate changes. But that depends as
> stated above on how the dfi-controller needs to be handled.
>
> For example the current armclk-handling uses a clock notifier around the
> actual rate change ... so you could use that as inspiration.
     Thank you for your inspiration. I think i can handle ddrc clk like 
the armclk.
     About the dfi, it will monitor ddr utilization, according this 
result to do
     ddr frequency scaling. I think i can implement a dfi drvier, and 
register it to devfreq,
     then call the dmc_set_rate fucntion in the dfi drvier, meanwhile i 
will implement a ddrc clk driver
     like the armclk, implement set rate funciton, and call the 
dcf(implement in ATF) in this function.
>
>
> Heiko
>
>
>
>

-- 
Lin Huang