* [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-09-20 9:57 ` Weiyi Lu
0 siblings, 0 replies; 26+ messages in thread
From: Weiyi Lu @ 2018-09-20 9:57 UTC (permalink / raw)
To: linux-arm-kernel
For some MT2712 projects, audpll could select another reference
clock source if there exists an extra Crystal Oscillators than
the default clk26m XTAL.
Declare with the property "mediatek,refclk-aud" to switch
the audpll reference clock.
And also support to modify the reference clock of all PLL with
property "mediatek,refclk" instead of the default source "clk26m".
Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
---
drivers/clk/mediatek/clk-mt2712.c | 87 ++++++++++++++++++++++++++++++---------
1 file changed, 68 insertions(+), 19 deletions(-)
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index e36f4aab634d..2a4db1718089 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1174,7 +1174,7 @@ static const struct mtk_gate peri_clks[] = {
#define PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, \
_pd_reg, _pd_shift, _tuner_reg, _tuner_en_reg, \
_tuner_en_bit, _pcw_reg, _pcw_shift, \
- _div_table) { \
+ _div_table, _parent_name) { \
.id = _id, \
.name = _name, \
.reg = _reg, \
@@ -1192,15 +1192,17 @@ static const struct mtk_gate peri_clks[] = {
.pcw_reg = _pcw_reg, \
.pcw_shift = _pcw_shift, \
.div_table = _div_table, \
+ .parent_name = _parent_name, \
}
#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, \
_pd_reg, _pd_shift, _tuner_reg, _tuner_en_reg, \
- _tuner_en_bit, _pcw_reg, _pcw_shift) \
+ _tuner_en_bit, _pcw_reg, _pcw_shift, \
+ _parent_name) \
PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, \
_pcwbits, _pd_reg, _pd_shift, _tuner_reg, \
_tuner_en_reg, _tuner_en_bit, _pcw_reg, \
- _pcw_shift, NULL)
+ _pcw_shift, NULL, _parent_name)
static const struct mtk_pll_div_table armca35pll_div_table[] = {
{ .div = 0, .freq = MT2712_PLL_FMAX },
@@ -1229,48 +1231,95 @@ static const struct mtk_pll_div_table mmpll_div_table[] = {
{ } /* sentinel */
};
-static const struct mtk_pll_data plls[] = {
+static struct mtk_pll_data plls[] = {
PLL(CLK_APMIXED_MAINPLL, "mainpll", 0x0230, 0x023C, 0xf0000101,
- HAVE_RST_BAR, 31, 0x0230, 4, 0, 0, 0, 0x0234, 0),
+ HAVE_RST_BAR, 31, 0x0230, 4, 0, 0, 0, 0x0234, 0, "clk26m"),
PLL(CLK_APMIXED_UNIVPLL, "univpll", 0x0240, 0x024C, 0xfe000101,
- HAVE_RST_BAR, 31, 0x0240, 4, 0, 0, 0, 0x0244, 0),
+ HAVE_RST_BAR, 31, 0x0240, 4, 0, 0, 0, 0x0244, 0, "clk26m"),
PLL(CLK_APMIXED_VCODECPLL, "vcodecpll", 0x0320, 0x032C, 0xc0000101,
- 0, 31, 0x0320, 4, 0, 0, 0, 0x0324, 0),
+ 0, 31, 0x0320, 4, 0, 0, 0, 0x0324, 0, "clk26m"),
PLL(CLK_APMIXED_VENCPLL, "vencpll", 0x0280, 0x028C, 0x00000101,
- 0, 31, 0x0280, 4, 0, 0, 0, 0x0284, 0),
+ 0, 31, 0x0280, 4, 0, 0, 0, 0x0284, 0, "clk26m"),
PLL(CLK_APMIXED_APLL1, "apll1", 0x0330, 0x0340, 0x00000101,
- 0, 31, 0x0330, 4, 0x0338, 0x0014, 0, 0x0334, 0),
+ 0, 31, 0x0330, 4, 0x0338, 0x0014, 0, 0x0334, 0, "clk26m"),
PLL(CLK_APMIXED_APLL2, "apll2", 0x0350, 0x0360, 0x00000101,
- 0, 31, 0x0350, 4, 0x0358, 0x0014, 1, 0x0354, 0),
+ 0, 31, 0x0350, 4, 0x0358, 0x0014, 1, 0x0354, 0, "clk26m"),
PLL(CLK_APMIXED_LVDSPLL, "lvdspll", 0x0370, 0x037c, 0x00000101,
- 0, 31, 0x0370, 4, 0, 0, 0, 0x0374, 0),
+ 0, 31, 0x0370, 4, 0, 0, 0, 0x0374, 0, "clk26m"),
PLL(CLK_APMIXED_LVDSPLL2, "lvdspll2", 0x0390, 0x039C, 0x00000101,
- 0, 31, 0x0390, 4, 0, 0, 0, 0x0394, 0),
+ 0, 31, 0x0390, 4, 0, 0, 0, 0x0394, 0, "clk26m"),
PLL(CLK_APMIXED_MSDCPLL, "msdcpll", 0x0270, 0x027C, 0x00000101,
- 0, 31, 0x0270, 4, 0, 0, 0, 0x0274, 0),
+ 0, 31, 0x0270, 4, 0, 0, 0, 0x0274, 0, "clk26m"),
PLL(CLK_APMIXED_MSDCPLL2, "msdcpll2", 0x0410, 0x041C, 0x00000101,
- 0, 31, 0x0410, 4, 0, 0, 0, 0x0414, 0),
+ 0, 31, 0x0410, 4, 0, 0, 0, 0x0414, 0, "clk26m"),
PLL(CLK_APMIXED_TVDPLL, "tvdpll", 0x0290, 0x029C, 0xc0000101,
- 0, 31, 0x0290, 4, 0, 0, 0, 0x0294, 0),
+ 0, 31, 0x0290, 4, 0, 0, 0, 0x0294, 0, "clk26m"),
PLL_B(CLK_APMIXED_MMPLL, "mmpll", 0x0250, 0x0260, 0x00000101,
0, 31, 0x0250, 4, 0, 0, 0, 0x0254, 0,
- mmpll_div_table),
+ mmpll_div_table, "clk26m"),
PLL_B(CLK_APMIXED_ARMCA35PLL, "armca35pll", 0x0100, 0x0110, 0xf0000101,
HAVE_RST_BAR, 31, 0x0100, 4, 0, 0, 0, 0x0104, 0,
- armca35pll_div_table),
+ armca35pll_div_table, "clk26m"),
PLL_B(CLK_APMIXED_ARMCA72PLL, "armca72pll", 0x0210, 0x0220, 0x00000101,
0, 31, 0x0210, 4, 0, 0, 0, 0x0214, 0,
- armca72pll_div_table),
+ armca72pll_div_table, "clk26m"),
PLL(CLK_APMIXED_ETHERPLL, "etherpll", 0x0300, 0x030C, 0xc0000101,
- 0, 31, 0x0300, 4, 0, 0, 0, 0x0304, 0),
+ 0, 31, 0x0300, 4, 0, 0, 0, 0x0304, 0, "clk26m"),
};
+static void clk_mt2712_set_pll_reference(struct device_node *node,
+ struct mtk_pll_data *plls, size_t num_plls)
+{
+ void __iomem *base, *sel_addr;
+ struct of_phandle_args refclk, refclk_aud;
+ const char *refclk_name = "clk26m";
+ const char *refclk_aud_name;
+ int rc;
+ size_t i;
+ u32 r;
+
+ base = of_iomap(node, 0);
+ if (base) {
+ sel_addr = base + 0x40;
+ } else {
+ pr_err("%s(): ioremap failed\n", __func__);
+ return;
+ }
+
+ rc = of_parse_phandle_with_args(node, "mediatek,refclk",
+ "#clock-cells", 0, &refclk);
+ if (!rc) {
+ of_property_read_string(refclk.np, "clock-output-names",
+ &refclk_name);
+ for (i = 0; i < num_plls; i++)
+ plls[i].parent_name = refclk_name;
+ }
+
+ rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
+ "#clock-cells", 0, &refclk_aud);
+ if (!rc) {
+ of_property_read_string(refclk_aud.np, "clock-output-names",
+ &refclk_aud_name);
+ if (strcmp(refclk_name, refclk_aud_name)) {
+ plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
+ plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
+ r = readl(sel_addr) | 0x60000;
+ } else {
+ r = readl(sel_addr) & ~0x60000;
+ }
+
+ writel(r, sel_addr);
+ }
+}
+
static int clk_mt2712_apmixed_probe(struct platform_device *pdev)
{
struct clk_onecell_data *clk_data;
int r;
struct device_node *node = pdev->dev.of_node;
+ clk_mt2712_set_pll_reference(node, plls, ARRAY_SIZE(plls));
+
clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
--
2.12.5.2.gbdf23ab
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-09-20 9:57 ` Weiyi Lu
0 siblings, 0 replies; 26+ messages in thread
From: Weiyi Lu @ 2018-09-20 9:57 UTC (permalink / raw)
To: Matthias Brugger, Stephen Boyd, Rob Herring
Cc: James Liao, Fan Chen, linux-arm-kernel, linux-kernel,
linux-mediatek, linux-clk, srv_heupstream, Weiyi Lu
For some MT2712 projects, audpll could select another reference
clock source if there exists an extra Crystal Oscillators than
the default clk26m XTAL.
Declare with the property "mediatek,refclk-aud" to switch
the audpll reference clock.
And also support to modify the reference clock of all PLL with
property "mediatek,refclk" instead of the default source "clk26m".
Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
---
drivers/clk/mediatek/clk-mt2712.c | 87 ++++++++++++++++++++++++++++++---------
1 file changed, 68 insertions(+), 19 deletions(-)
diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
index e36f4aab634d..2a4db1718089 100644
--- a/drivers/clk/mediatek/clk-mt2712.c
+++ b/drivers/clk/mediatek/clk-mt2712.c
@@ -1174,7 +1174,7 @@ static const struct mtk_gate peri_clks[] = {
#define PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, \
_pd_reg, _pd_shift, _tuner_reg, _tuner_en_reg, \
_tuner_en_bit, _pcw_reg, _pcw_shift, \
- _div_table) { \
+ _div_table, _parent_name) { \
.id = _id, \
.name = _name, \
.reg = _reg, \
@@ -1192,15 +1192,17 @@ static const struct mtk_gate peri_clks[] = {
.pcw_reg = _pcw_reg, \
.pcw_shift = _pcw_shift, \
.div_table = _div_table, \
+ .parent_name = _parent_name, \
}
#define PLL(_id, _name, _reg, _pwr_reg, _en_mask, _flags, _pcwbits, \
_pd_reg, _pd_shift, _tuner_reg, _tuner_en_reg, \
- _tuner_en_bit, _pcw_reg, _pcw_shift) \
+ _tuner_en_bit, _pcw_reg, _pcw_shift, \
+ _parent_name) \
PLL_B(_id, _name, _reg, _pwr_reg, _en_mask, _flags, \
_pcwbits, _pd_reg, _pd_shift, _tuner_reg, \
_tuner_en_reg, _tuner_en_bit, _pcw_reg, \
- _pcw_shift, NULL)
+ _pcw_shift, NULL, _parent_name)
static const struct mtk_pll_div_table armca35pll_div_table[] = {
{ .div = 0, .freq = MT2712_PLL_FMAX },
@@ -1229,48 +1231,95 @@ static const struct mtk_pll_div_table mmpll_div_table[] = {
{ } /* sentinel */
};
-static const struct mtk_pll_data plls[] = {
+static struct mtk_pll_data plls[] = {
PLL(CLK_APMIXED_MAINPLL, "mainpll", 0x0230, 0x023C, 0xf0000101,
- HAVE_RST_BAR, 31, 0x0230, 4, 0, 0, 0, 0x0234, 0),
+ HAVE_RST_BAR, 31, 0x0230, 4, 0, 0, 0, 0x0234, 0, "clk26m"),
PLL(CLK_APMIXED_UNIVPLL, "univpll", 0x0240, 0x024C, 0xfe000101,
- HAVE_RST_BAR, 31, 0x0240, 4, 0, 0, 0, 0x0244, 0),
+ HAVE_RST_BAR, 31, 0x0240, 4, 0, 0, 0, 0x0244, 0, "clk26m"),
PLL(CLK_APMIXED_VCODECPLL, "vcodecpll", 0x0320, 0x032C, 0xc0000101,
- 0, 31, 0x0320, 4, 0, 0, 0, 0x0324, 0),
+ 0, 31, 0x0320, 4, 0, 0, 0, 0x0324, 0, "clk26m"),
PLL(CLK_APMIXED_VENCPLL, "vencpll", 0x0280, 0x028C, 0x00000101,
- 0, 31, 0x0280, 4, 0, 0, 0, 0x0284, 0),
+ 0, 31, 0x0280, 4, 0, 0, 0, 0x0284, 0, "clk26m"),
PLL(CLK_APMIXED_APLL1, "apll1", 0x0330, 0x0340, 0x00000101,
- 0, 31, 0x0330, 4, 0x0338, 0x0014, 0, 0x0334, 0),
+ 0, 31, 0x0330, 4, 0x0338, 0x0014, 0, 0x0334, 0, "clk26m"),
PLL(CLK_APMIXED_APLL2, "apll2", 0x0350, 0x0360, 0x00000101,
- 0, 31, 0x0350, 4, 0x0358, 0x0014, 1, 0x0354, 0),
+ 0, 31, 0x0350, 4, 0x0358, 0x0014, 1, 0x0354, 0, "clk26m"),
PLL(CLK_APMIXED_LVDSPLL, "lvdspll", 0x0370, 0x037c, 0x00000101,
- 0, 31, 0x0370, 4, 0, 0, 0, 0x0374, 0),
+ 0, 31, 0x0370, 4, 0, 0, 0, 0x0374, 0, "clk26m"),
PLL(CLK_APMIXED_LVDSPLL2, "lvdspll2", 0x0390, 0x039C, 0x00000101,
- 0, 31, 0x0390, 4, 0, 0, 0, 0x0394, 0),
+ 0, 31, 0x0390, 4, 0, 0, 0, 0x0394, 0, "clk26m"),
PLL(CLK_APMIXED_MSDCPLL, "msdcpll", 0x0270, 0x027C, 0x00000101,
- 0, 31, 0x0270, 4, 0, 0, 0, 0x0274, 0),
+ 0, 31, 0x0270, 4, 0, 0, 0, 0x0274, 0, "clk26m"),
PLL(CLK_APMIXED_MSDCPLL2, "msdcpll2", 0x0410, 0x041C, 0x00000101,
- 0, 31, 0x0410, 4, 0, 0, 0, 0x0414, 0),
+ 0, 31, 0x0410, 4, 0, 0, 0, 0x0414, 0, "clk26m"),
PLL(CLK_APMIXED_TVDPLL, "tvdpll", 0x0290, 0x029C, 0xc0000101,
- 0, 31, 0x0290, 4, 0, 0, 0, 0x0294, 0),
+ 0, 31, 0x0290, 4, 0, 0, 0, 0x0294, 0, "clk26m"),
PLL_B(CLK_APMIXED_MMPLL, "mmpll", 0x0250, 0x0260, 0x00000101,
0, 31, 0x0250, 4, 0, 0, 0, 0x0254, 0,
- mmpll_div_table),
+ mmpll_div_table, "clk26m"),
PLL_B(CLK_APMIXED_ARMCA35PLL, "armca35pll", 0x0100, 0x0110, 0xf0000101,
HAVE_RST_BAR, 31, 0x0100, 4, 0, 0, 0, 0x0104, 0,
- armca35pll_div_table),
+ armca35pll_div_table, "clk26m"),
PLL_B(CLK_APMIXED_ARMCA72PLL, "armca72pll", 0x0210, 0x0220, 0x00000101,
0, 31, 0x0210, 4, 0, 0, 0, 0x0214, 0,
- armca72pll_div_table),
+ armca72pll_div_table, "clk26m"),
PLL(CLK_APMIXED_ETHERPLL, "etherpll", 0x0300, 0x030C, 0xc0000101,
- 0, 31, 0x0300, 4, 0, 0, 0, 0x0304, 0),
+ 0, 31, 0x0300, 4, 0, 0, 0, 0x0304, 0, "clk26m"),
};
+static void clk_mt2712_set_pll_reference(struct device_node *node,
+ struct mtk_pll_data *plls, size_t num_plls)
+{
+ void __iomem *base, *sel_addr;
+ struct of_phandle_args refclk, refclk_aud;
+ const char *refclk_name = "clk26m";
+ const char *refclk_aud_name;
+ int rc;
+ size_t i;
+ u32 r;
+
+ base = of_iomap(node, 0);
+ if (base) {
+ sel_addr = base + 0x40;
+ } else {
+ pr_err("%s(): ioremap failed\n", __func__);
+ return;
+ }
+
+ rc = of_parse_phandle_with_args(node, "mediatek,refclk",
+ "#clock-cells", 0, &refclk);
+ if (!rc) {
+ of_property_read_string(refclk.np, "clock-output-names",
+ &refclk_name);
+ for (i = 0; i < num_plls; i++)
+ plls[i].parent_name = refclk_name;
+ }
+
+ rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
+ "#clock-cells", 0, &refclk_aud);
+ if (!rc) {
+ of_property_read_string(refclk_aud.np, "clock-output-names",
+ &refclk_aud_name);
+ if (strcmp(refclk_name, refclk_aud_name)) {
+ plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
+ plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
+ r = readl(sel_addr) | 0x60000;
+ } else {
+ r = readl(sel_addr) & ~0x60000;
+ }
+
+ writel(r, sel_addr);
+ }
+}
+
static int clk_mt2712_apmixed_probe(struct platform_device *pdev)
{
struct clk_onecell_data *clk_data;
int r;
struct device_node *node = pdev->dev.of_node;
+ clk_mt2712_set_pll_reference(node, plls, ARRAY_SIZE(plls));
+
clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK);
mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
--
2.12.5.2.gbdf23ab
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
2018-09-20 9:57 ` Weiyi Lu
(?)
@ 2018-10-12 17:53 ` Stephen Boyd
-1 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-12 17:53 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Stephen Boyd, Weiyi Lu
Cc: James Liao, Fan Chen, linux-arm-kernel, linux-kernel,
linux-mediatek, linux-clk, srv_heupstream, Weiyi Lu
Quoting Weiyi Lu (2018-09-20 02:57:27)
> For some MT2712 projects, audpll could select another reference
> clock source if there exists an extra Crystal Oscillators than
> the default clk26m XTAL.
> Declare with the property "mediatek,refclk-aud" to switch
> the audpll reference clock.
> And also support to modify the reference clock of all PLL with
> property "mediatek,refclk" instead of the default source "clk26m".
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
> diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
> index e36f4aab634d..2a4db1718089 100644
> --- a/drivers/clk/mediatek/clk-mt2712.c
> +++ b/drivers/clk/mediatek/clk-mt2712.c
[...]
> + size_t i;
> + u32 r;
> +
> + base = of_iomap(node, 0);
> + if (base) {
> + sel_addr = base + 0x40;
> + } else {
> + pr_err("%s(): ioremap failed\n", __func__);
> + return;
> + }
Nitpick: Write this as
base = of_iomap();
if (!base)
return;
sel_addr = base + 0x40;
> +
> + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> + "#clock-cells", 0, &refclk);
> + if (!rc) {
> + of_property_read_string(refclk.np, "clock-output-names",
> + &refclk_name);
> + for (i = 0; i < num_plls; i++)
> + plls[i].parent_name = refclk_name;
Use of_clk_parent_fill()?
> + }
> +
> + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> + "#clock-cells", 0, &refclk_aud);
This is odd. Is this a custom 'clocks' property? What's going on here?
Why can't we use assigned clock parents for this?
> + if (!rc) {
> + of_property_read_string(refclk_aud.np, "clock-output-names",
> + &refclk_aud_name);
> + if (strcmp(refclk_name, refclk_aud_name)) {
> + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
> + plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
> + r = readl(sel_addr) | 0x60000;
> + } else {
> + r = readl(sel_addr) & ~0x60000;
> + }
> +
> + writel(r, sel_addr);
> + }
> +}
> +
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-10-12 17:53 ` Stephen Boyd
0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-12 17:53 UTC (permalink / raw)
To: linux-arm-kernel
Quoting Weiyi Lu (2018-09-20 02:57:27)
> For some MT2712 projects, audpll could select another reference
> clock source if there exists an extra Crystal Oscillators than
> the default clk26m XTAL.
> Declare with the property "mediatek,refclk-aud" to switch
> the audpll reference clock.
> And also support to modify the reference clock of all PLL with
> property "mediatek,refclk" instead of the default source "clk26m".
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
> diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
> index e36f4aab634d..2a4db1718089 100644
> --- a/drivers/clk/mediatek/clk-mt2712.c
> +++ b/drivers/clk/mediatek/clk-mt2712.c
[...]
> + size_t i;
> + u32 r;
> +
> + base = of_iomap(node, 0);
> + if (base) {
> + sel_addr = base + 0x40;
> + } else {
> + pr_err("%s(): ioremap failed\n", __func__);
> + return;
> + }
Nitpick: Write this as
base = of_iomap();
if (!base)
return;
sel_addr = base + 0x40;
> +
> + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> + "#clock-cells", 0, &refclk);
> + if (!rc) {
> + of_property_read_string(refclk.np, "clock-output-names",
> + &refclk_name);
> + for (i = 0; i < num_plls; i++)
> + plls[i].parent_name = refclk_name;
Use of_clk_parent_fill()?
> + }
> +
> + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> + "#clock-cells", 0, &refclk_aud);
This is odd. Is this a custom 'clocks' property? What's going on here?
Why can't we use assigned clock parents for this?
> + if (!rc) {
> + of_property_read_string(refclk_aud.np, "clock-output-names",
> + &refclk_aud_name);
> + if (strcmp(refclk_name, refclk_aud_name)) {
> + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
> + plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
> + r = readl(sel_addr) | 0x60000;
> + } else {
> + r = readl(sel_addr) & ~0x60000;
> + }
> +
> + writel(r, sel_addr);
> + }
> +}
> +
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-10-12 17:53 ` Stephen Boyd
0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-12 17:53 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Stephen Boyd
Cc: James Liao, Fan Chen, linux-arm-kernel, linux-kernel,
linux-mediatek, linux-clk, srv_heupstream, Weiyi Lu
Quoting Weiyi Lu (2018-09-20 02:57:27)
> For some MT2712 projects, audpll could select another reference
> clock source if there exists an extra Crystal Oscillators than
> the default clk26m XTAL.
> Declare with the property "mediatek,refclk-aud" to switch
> the audpll reference clock.
> And also support to modify the reference clock of all PLL with
> property "mediatek,refclk" instead of the default source "clk26m".
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
> diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
> index e36f4aab634d..2a4db1718089 100644
> --- a/drivers/clk/mediatek/clk-mt2712.c
> +++ b/drivers/clk/mediatek/clk-mt2712.c
[...]
> + size_t i;
> + u32 r;
> +
> + base = of_iomap(node, 0);
> + if (base) {
> + sel_addr = base + 0x40;
> + } else {
> + pr_err("%s(): ioremap failed\n", __func__);
> + return;
> + }
Nitpick: Write this as
base = of_iomap();
if (!base)
return;
sel_addr = base + 0x40;
> +
> + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> + "#clock-cells", 0, &refclk);
> + if (!rc) {
> + of_property_read_string(refclk.np, "clock-output-names",
> + &refclk_name);
> + for (i = 0; i < num_plls; i++)
> + plls[i].parent_name = refclk_name;
Use of_clk_parent_fill()?
> + }
> +
> + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> + "#clock-cells", 0, &refclk_aud);
This is odd. Is this a custom 'clocks' property? What's going on here?
Why can't we use assigned clock parents for this?
> + if (!rc) {
> + of_property_read_string(refclk_aud.np, "clock-output-names",
> + &refclk_aud_name);
> + if (strcmp(refclk_name, refclk_aud_name)) {
> + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
> + plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
> + r = readl(sel_addr) | 0x60000;
> + } else {
> + r = readl(sel_addr) & ~0x60000;
> + }
> +
> + writel(r, sel_addr);
> + }
> +}
> +
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
2018-10-12 17:53 ` Stephen Boyd
@ 2018-10-17 13:53 ` Weiyi Lu
-1 siblings, 0 replies; 26+ messages in thread
From: Weiyi Lu @ 2018-10-17 13:53 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, srv_heupstream, James Liao, Stephen Boyd,
linux-kernel, Fan Chen, linux-mediatek, Matthias Brugger,
linux-clk, linux-arm-kernel
On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> Quoting Weiyi Lu (2018-09-20 02:57:27)
> > For some MT2712 projects, audpll could select another reference
> > clock source if there exists an extra Crystal Oscillators than
> > the default clk26m XTAL.
> > Declare with the property "mediatek,refclk-aud" to switch
> > the audpll reference clock.
> > And also support to modify the reference clock of all PLL with
> > property "mediatek,refclk" instead of the default source "clk26m".
> >
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > ---
> > diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
> > index e36f4aab634d..2a4db1718089 100644
> > --- a/drivers/clk/mediatek/clk-mt2712.c
> > +++ b/drivers/clk/mediatek/clk-mt2712.c
> [...]
> > + size_t i;
> > + u32 r;
> > +
> > + base = of_iomap(node, 0);
> > + if (base) {
> > + sel_addr = base + 0x40;
> > + } else {
> > + pr_err("%s(): ioremap failed\n", __func__);
> > + return;
> > + }
>
> Nitpick: Write this as
>
> base = of_iomap();
> if (!base)
> return;
> sel_addr = base + 0x40;
>
Got it, I'll fix it.
> > +
> > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > + "#clock-cells", 0, &refclk);
> > + if (!rc) {
> > + of_property_read_string(refclk.np, "clock-output-names",
> > + &refclk_name);
> > + for (i = 0; i < num_plls; i++)
> > + plls[i].parent_name = refclk_name;
>
> Use of_clk_parent_fill()?
>
Thanks for the hint, i might use of_clk_get_parent_name() instead like
below to get each parent clock name.
refclk_name = of_clk_get_parent_name(node, 0);
refclk_aud_name = of_clk_get_parent_name(node, 1);
> > + }
> > +
> > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > + "#clock-cells", 0, &refclk_aud);
>
> This is odd. Is this a custom 'clocks' property? What's going on here?
> Why can't we use assigned clock parents for this?
Yes. both the reference clock of all PLL and the reference clock of
audio PLL could be customized.These two reference clocks shall be
provided by some component like crystal oscillators on the board. So we
might unable to switch the PLL reference source at runtime, in other
words, it should be set statically. That's why we didn't provide the
set_parent ops for pll clock type. It's also the main reason we choose
not to use assigned-clock-parents for this requirement.
> > + if (!rc) {
> > + of_property_read_string(refclk_aud.np, "clock-output-names",
> > + &refclk_aud_name);
> > + if (strcmp(refclk_name, refclk_aud_name)) {
> > + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
> > + plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
> > + r = readl(sel_addr) | 0x60000;
> > + } else {
> > + r = readl(sel_addr) & ~0x60000;
> > + }
> > +
> > + writel(r, sel_addr);
> > + }
> > +}
> > +
^ permalink raw reply [flat|nested] 26+ messages in thread* [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-10-17 13:53 ` Weiyi Lu
0 siblings, 0 replies; 26+ messages in thread
From: Weiyi Lu @ 2018-10-17 13:53 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> Quoting Weiyi Lu (2018-09-20 02:57:27)
> > For some MT2712 projects, audpll could select another reference
> > clock source if there exists an extra Crystal Oscillators than
> > the default clk26m XTAL.
> > Declare with the property "mediatek,refclk-aud" to switch
> > the audpll reference clock.
> > And also support to modify the reference clock of all PLL with
> > property "mediatek,refclk" instead of the default source "clk26m".
> >
> > Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> > ---
> > diff --git a/drivers/clk/mediatek/clk-mt2712.c b/drivers/clk/mediatek/clk-mt2712.c
> > index e36f4aab634d..2a4db1718089 100644
> > --- a/drivers/clk/mediatek/clk-mt2712.c
> > +++ b/drivers/clk/mediatek/clk-mt2712.c
> [...]
> > + size_t i;
> > + u32 r;
> > +
> > + base = of_iomap(node, 0);
> > + if (base) {
> > + sel_addr = base + 0x40;
> > + } else {
> > + pr_err("%s(): ioremap failed\n", __func__);
> > + return;
> > + }
>
> Nitpick: Write this as
>
> base = of_iomap();
> if (!base)
> return;
> sel_addr = base + 0x40;
>
Got it, I'll fix it.
> > +
> > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > + "#clock-cells", 0, &refclk);
> > + if (!rc) {
> > + of_property_read_string(refclk.np, "clock-output-names",
> > + &refclk_name);
> > + for (i = 0; i < num_plls; i++)
> > + plls[i].parent_name = refclk_name;
>
> Use of_clk_parent_fill()?
>
Thanks for the hint, i might use of_clk_get_parent_name() instead like
below to get each parent clock name.
refclk_name = of_clk_get_parent_name(node, 0);
refclk_aud_name = of_clk_get_parent_name(node, 1);
> > + }
> > +
> > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > + "#clock-cells", 0, &refclk_aud);
>
> This is odd. Is this a custom 'clocks' property? What's going on here?
> Why can't we use assigned clock parents for this?
Yes. both the reference clock of all PLL and the reference clock of
audio PLL could be customized.These two reference clocks shall be
provided by some component like crystal oscillators on the board. So we
might unable to switch the PLL reference source at runtime, in other
words, it should be set statically. That's why we didn't provide the
set_parent ops for pll clock type. It's also the main reason we choose
not to use assigned-clock-parents for this requirement.
> > + if (!rc) {
> > + of_property_read_string(refclk_aud.np, "clock-output-names",
> > + &refclk_aud_name);
> > + if (strcmp(refclk_name, refclk_aud_name)) {
> > + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
> > + plls[CLK_APMIXED_APLL2].parent_name = refclk_aud_name;
> > + r = readl(sel_addr) | 0x60000;
> > + } else {
> > + r = readl(sel_addr) & ~0x60000;
> > + }
> > +
> > + writel(r, sel_addr);
> > + }
> > +}
> > +
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
2018-10-17 13:53 ` Weiyi Lu
@ 2018-10-17 14:15 ` Stephen Boyd
-1 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-17 14:15 UTC (permalink / raw)
To: Weiyi Lu
Cc: Rob Herring, srv_heupstream, James Liao, Stephen Boyd,
linux-kernel, Fan Chen, linux-mediatek, Matthias Brugger,
linux-clk, linux-arm-kernel
Quoting Weiyi Lu (2018-10-17 06:53:12)
> On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> > Quoting Weiyi Lu (2018-09-20 02:57:27)
> > > +
> > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > > + "#clock-cells", 0, &refclk);
> > > + if (!rc) {
> > > + of_property_read_string(refclk.np, "clock-output-names",
> > > + &refclk_name);
> > > + for (i = 0; i < num_plls; i++)
> > > + plls[i].parent_name = refclk_name;
> >
> > Use of_clk_parent_fill()?
> >
> Thanks for the hint, i might use of_clk_get_parent_name() instead like
> below to get each parent clock name.
> refclk_name = of_clk_get_parent_name(node, 0);
> refclk_aud_name = of_clk_get_parent_name(node, 1);
Alright.
>
> > > + }
> > > +
> > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > > + "#clock-cells", 0, &refclk_aud);
> >
> > This is odd. Is this a custom 'clocks' property? What's going on here?
> > Why can't we use assigned clock parents for this?
> Yes. both the reference clock of all PLL and the reference clock of
> audio PLL could be customized.These two reference clocks shall be
> provided by some component like crystal oscillators on the board. So we
> might unable to switch the PLL reference source at runtime, in other
> words, it should be set statically. That's why we didn't provide the
> set_parent ops for pll clock type. It's also the main reason we choose
> not to use assigned-clock-parents for this requirement.
>
Ok. Do you need some new software clk flag that indicates we should
"lock" the parent or rate configured at boot time? I would like to see
this driver implement the clk_ops for the hardware and have the
framework be the one that configures the tree, instead of doing it some
custom way for this single driver.
> > > + if (!rc) {
> > > + of_property_read_string(refclk_aud.np, "clock-output-names",
> > > + &refclk_aud_name);
> > > + if (strcmp(refclk_name, refclk_aud_name)) {
> > > + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
^ permalink raw reply [flat|nested] 26+ messages in thread* [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-10-17 14:15 ` Stephen Boyd
0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-17 14:15 UTC (permalink / raw)
To: linux-arm-kernel
Quoting Weiyi Lu (2018-10-17 06:53:12)
> On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> > Quoting Weiyi Lu (2018-09-20 02:57:27)
> > > +
> > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > > + "#clock-cells", 0, &refclk);
> > > + if (!rc) {
> > > + of_property_read_string(refclk.np, "clock-output-names",
> > > + &refclk_name);
> > > + for (i = 0; i < num_plls; i++)
> > > + plls[i].parent_name = refclk_name;
> >
> > Use of_clk_parent_fill()?
> >
> Thanks for the hint, i might use of_clk_get_parent_name() instead like
> below to get each parent clock name.
> refclk_name = of_clk_get_parent_name(node, 0);
> refclk_aud_name = of_clk_get_parent_name(node, 1);
Alright.
>
> > > + }
> > > +
> > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > > + "#clock-cells", 0, &refclk_aud);
> >
> > This is odd. Is this a custom 'clocks' property? What's going on here?
> > Why can't we use assigned clock parents for this?
> Yes. both the reference clock of all PLL and the reference clock of
> audio PLL could be customized.These two reference clocks shall be
> provided by some component like crystal oscillators on the board. So we
> might unable to switch the PLL reference source at runtime, in other
> words, it should be set statically. That's why we didn't provide the
> set_parent ops for pll clock type. It's also the main reason we choose
> not to use assigned-clock-parents for this requirement.
>
Ok. Do you need some new software clk flag that indicates we should
"lock" the parent or rate configured at boot time? I would like to see
this driver implement the clk_ops for the hardware and have the
framework be the one that configures the tree, instead of doing it some
custom way for this single driver.
> > > + if (!rc) {
> > > + of_property_read_string(refclk_aud.np, "clock-output-names",
> > > + &refclk_aud_name);
> > > + if (strcmp(refclk_name, refclk_aud_name)) {
> > > + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
2018-10-17 14:15 ` Stephen Boyd
@ 2018-10-18 6:00 ` Weiyi Lu
-1 siblings, 0 replies; 26+ messages in thread
From: Weiyi Lu @ 2018-10-18 6:00 UTC (permalink / raw)
To: Stephen Boyd
Cc: Rob Herring, srv_heupstream, James Liao, Stephen Boyd,
linux-kernel, Fan Chen, linux-mediatek, Matthias Brugger,
linux-clk, linux-arm-kernel
On Wed, 2018-10-17 at 07:15 -0700, Stephen Boyd wrote:
> Quoting Weiyi Lu (2018-10-17 06:53:12)
> > On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> > > Quoting Weiyi Lu (2018-09-20 02:57:27)
> > > > +
> > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > > > + "#clock-cells", 0, &refclk);
> > > > + if (!rc) {
> > > > + of_property_read_string(refclk.np, "clock-output-names",
> > > > + &refclk_name);
> > > > + for (i = 0; i < num_plls; i++)
> > > > + plls[i].parent_name = refclk_name;
> > >
> > > Use of_clk_parent_fill()?
> > >
> > Thanks for the hint, i might use of_clk_get_parent_name() instead like
> > below to get each parent clock name.
> > refclk_name = of_clk_get_parent_name(node, 0);
> > refclk_aud_name = of_clk_get_parent_name(node, 1);
>
> Alright.
>
> >
> > > > + }
> > > > +
> > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > > > + "#clock-cells", 0, &refclk_aud);
> > >
> > > This is odd. Is this a custom 'clocks' property? What's going on here?
> > > Why can't we use assigned clock parents for this?
> > Yes. both the reference clock of all PLL and the reference clock of
> > audio PLL could be customized.These two reference clocks shall be
> > provided by some component like crystal oscillators on the board. So we
> > might unable to switch the PLL reference source at runtime, in other
> > words, it should be set statically. That's why we didn't provide the
> > set_parent ops for pll clock type. It's also the main reason we choose
> > not to use assigned-clock-parents for this requirement.
> >
>
> Ok. Do you need some new software clk flag that indicates we should
> "lock" the parent or rate configured at boot time? I would like to see
> this driver implement the clk_ops for the hardware and have the
> framework be the one that configures the tree, instead of doing it some
> custom way for this single driver.
>
Hi Stephen, do you mean we might be able to add a flag just like
CLK_IS_CRITICAL and how it works on clk_enable & clk_disable ops?
And should we make this new flag to allow to set parent before clock is
registered and prohibit from changing the rate and parent after clock
registration. If my understanding is correct, I'll give a try.
BTW, how about the two patch for the ECO change before this patch. Is
there any problem on those two patches?
> > > > + if (!rc) {
> > > > + of_property_read_string(refclk_aud.np, "clock-output-names",
> > > > + &refclk_aud_name);
> > > > + if (strcmp(refclk_name, refclk_aud_name)) {
> > > > + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
^ permalink raw reply [flat|nested] 26+ messages in thread* [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-10-18 6:00 ` Weiyi Lu
0 siblings, 0 replies; 26+ messages in thread
From: Weiyi Lu @ 2018-10-18 6:00 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 2018-10-17 at 07:15 -0700, Stephen Boyd wrote:
> Quoting Weiyi Lu (2018-10-17 06:53:12)
> > On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> > > Quoting Weiyi Lu (2018-09-20 02:57:27)
> > > > +
> > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > > > + "#clock-cells", 0, &refclk);
> > > > + if (!rc) {
> > > > + of_property_read_string(refclk.np, "clock-output-names",
> > > > + &refclk_name);
> > > > + for (i = 0; i < num_plls; i++)
> > > > + plls[i].parent_name = refclk_name;
> > >
> > > Use of_clk_parent_fill()?
> > >
> > Thanks for the hint, i might use of_clk_get_parent_name() instead like
> > below to get each parent clock name.
> > refclk_name = of_clk_get_parent_name(node, 0);
> > refclk_aud_name = of_clk_get_parent_name(node, 1);
>
> Alright.
>
> >
> > > > + }
> > > > +
> > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > > > + "#clock-cells", 0, &refclk_aud);
> > >
> > > This is odd. Is this a custom 'clocks' property? What's going on here?
> > > Why can't we use assigned clock parents for this?
> > Yes. both the reference clock of all PLL and the reference clock of
> > audio PLL could be customized.These two reference clocks shall be
> > provided by some component like crystal oscillators on the board. So we
> > might unable to switch the PLL reference source at runtime, in other
> > words, it should be set statically. That's why we didn't provide the
> > set_parent ops for pll clock type. It's also the main reason we choose
> > not to use assigned-clock-parents for this requirement.
> >
>
> Ok. Do you need some new software clk flag that indicates we should
> "lock" the parent or rate configured at boot time? I would like to see
> this driver implement the clk_ops for the hardware and have the
> framework be the one that configures the tree, instead of doing it some
> custom way for this single driver.
>
Hi Stephen, do you mean we might be able to add a flag just like
CLK_IS_CRITICAL and how it works on clk_enable & clk_disable ops?
And should we make this new flag to allow to set parent before clock is
registered and prohibit from changing the rate and parent after clock
registration. If my understanding is correct, I'll give a try.
BTW, how about the two patch for the ECO change before this patch. Is
there any problem on those two patches?
> > > > + if (!rc) {
> > > > + of_property_read_string(refclk_aud.np, "clock-output-names",
> > > > + &refclk_aud_name);
> > > > + if (strcmp(refclk_name, refclk_aud_name)) {
> > > > + plls[CLK_APMIXED_APLL1].parent_name = refclk_aud_name;
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
2018-10-18 6:00 ` Weiyi Lu
@ 2018-10-18 17:05 ` Stephen Boyd
-1 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-18 17:05 UTC (permalink / raw)
To: Weiyi Lu
Cc: Rob Herring, srv_heupstream, James Liao, Stephen Boyd,
linux-kernel, Fan Chen, linux-mediatek, Matthias Brugger,
linux-clk, linux-arm-kernel
Quoting Weiyi Lu (2018-10-17 23:00:17)
> On Wed, 2018-10-17 at 07:15 -0700, Stephen Boyd wrote:
> > Quoting Weiyi Lu (2018-10-17 06:53:12)
> > > On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> > > > Quoting Weiyi Lu (2018-09-20 02:57:27)
> > > > > +
> > > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > > > > + "#clock-cells", 0, &refclk);
> > > > > + if (!rc) {
> > > > > + of_property_read_string(refclk.np, "clock-output-names",
> > > > > + &refclk_name);
> > > > > + for (i = 0; i < num_plls; i++)
> > > > > + plls[i].parent_name = refclk_name;
> > > >
> > > > Use of_clk_parent_fill()?
> > > >
> > > Thanks for the hint, i might use of_clk_get_parent_name() instead like
> > > below to get each parent clock name.
> > > refclk_name = of_clk_get_parent_name(node, 0);
> > > refclk_aud_name = of_clk_get_parent_name(node, 1);
> >
> > Alright.
> >
> > >
> > > > > + }
> > > > > +
> > > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > > > > + "#clock-cells", 0, &refclk_aud);
> > > >
> > > > This is odd. Is this a custom 'clocks' property? What's going on here?
> > > > Why can't we use assigned clock parents for this?
> > > Yes. both the reference clock of all PLL and the reference clock of
> > > audio PLL could be customized.These two reference clocks shall be
> > > provided by some component like crystal oscillators on the board. So we
> > > might unable to switch the PLL reference source at runtime, in other
> > > words, it should be set statically. That's why we didn't provide the
> > > set_parent ops for pll clock type. It's also the main reason we choose
> > > not to use assigned-clock-parents for this requirement.
> > >
> >
> > Ok. Do you need some new software clk flag that indicates we should
> > "lock" the parent or rate configured at boot time? I would like to see
> > this driver implement the clk_ops for the hardware and have the
> > framework be the one that configures the tree, instead of doing it some
> > custom way for this single driver.
> >
> Hi Stephen, do you mean we might be able to add a flag just like
> CLK_IS_CRITICAL and how it works on clk_enable & clk_disable ops?
> And should we make this new flag to allow to set parent before clock is
> registered and prohibit from changing the rate and parent after clock
> registration. If my understanding is correct, I'll give a try.
Mostly, yes. It would be similar to CLK_IS_CRITICAL, like
CLK_CONFIGURE_ONCE or something like that. It would effectively lock the
configuration after the clk is registered and any assigned parents or
rates are applied.
> BTW, how about the two patch for the ECO change before this patch. Is
> there any problem on those two patches?
>
No, those look fine. I just thought you would resend the whole series
again.
^ permalink raw reply [flat|nested] 26+ messages in thread* [SPAM]Re: [PATCH v1 3/3] clk: mediatek: mt2712: add pll reference support
@ 2018-10-18 17:05 ` Stephen Boyd
0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-10-18 17:05 UTC (permalink / raw)
To: linux-arm-kernel
Quoting Weiyi Lu (2018-10-17 23:00:17)
> On Wed, 2018-10-17 at 07:15 -0700, Stephen Boyd wrote:
> > Quoting Weiyi Lu (2018-10-17 06:53:12)
> > > On Fri, 2018-10-12 at 10:53 -0700, Stephen Boyd wrote:
> > > > Quoting Weiyi Lu (2018-09-20 02:57:27)
> > > > > +
> > > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk",
> > > > > + "#clock-cells", 0, &refclk);
> > > > > + if (!rc) {
> > > > > + of_property_read_string(refclk.np, "clock-output-names",
> > > > > + &refclk_name);
> > > > > + for (i = 0; i < num_plls; i++)
> > > > > + plls[i].parent_name = refclk_name;
> > > >
> > > > Use of_clk_parent_fill()?
> > > >
> > > Thanks for the hint, i might use of_clk_get_parent_name() instead like
> > > below to get each parent clock name.
> > > refclk_name = of_clk_get_parent_name(node, 0);
> > > refclk_aud_name = of_clk_get_parent_name(node, 1);
> >
> > Alright.
> >
> > >
> > > > > + }
> > > > > +
> > > > > + rc = of_parse_phandle_with_args(node, "mediatek,refclk-aud",
> > > > > + "#clock-cells", 0, &refclk_aud);
> > > >
> > > > This is odd. Is this a custom 'clocks' property? What's going on here?
> > > > Why can't we use assigned clock parents for this?
> > > Yes. both the reference clock of all PLL and the reference clock of
> > > audio PLL could be customized.These two reference clocks shall be
> > > provided by some component like crystal oscillators on the board. So we
> > > might unable to switch the PLL reference source at runtime, in other
> > > words, it should be set statically. That's why we didn't provide the
> > > set_parent ops for pll clock type. It's also the main reason we choose
> > > not to use assigned-clock-parents for this requirement.
> > >
> >
> > Ok. Do you need some new software clk flag that indicates we should
> > "lock" the parent or rate configured at boot time? I would like to see
> > this driver implement the clk_ops for the hardware and have the
> > framework be the one that configures the tree, instead of doing it some
> > custom way for this single driver.
> >
> Hi Stephen, do you mean we might be able to add a flag just like
> CLK_IS_CRITICAL and how it works on clk_enable & clk_disable ops?
> And should we make this new flag to allow to set parent before clock is
> registered and prohibit from changing the rate and parent after clock
> registration. If my understanding is correct, I'll give a try.
Mostly, yes. It would be similar to CLK_IS_CRITICAL, like
CLK_CONFIGURE_ONCE or something like that. It would effectively lock the
configuration after the clk is registered and any assigned parents or
rates are applied.
> BTW, how about the two patch for the ECO change before this patch. Is
> there any problem on those two patches?
>
No, those look fine. I just thought you would resend the whole series
again.
^ permalink raw reply [flat|nested] 26+ messages in thread