<div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace">Hi Stephan,</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">Thanks for all the comments!</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">This patch include one change so it's one commit. </div><div class="gmail_default" style="font-family:monospace,monospace">This clock module includes 4 PLLs and then a tree of muxes and dividers. </div><div class="gmail_default" style="font-family:monospace,monospace">All muxes and dividers are preset before Linux boots. The presetting can change according to the board. </div><div class="gmail_default" style="font-family:monospace,monospace">Linux drivers need to know what the frequencies are, but they can not change it, </div><div class="gmail_default" style="font-family:monospace,monospace">so this whole driver is intended as read only mechanism for clocks. </div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">On the first version which was up-streamed the PLLs input clk value was defined inside the driver.</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace">After the review I was asked that the fixed clock will be on the DT. This is this fix.</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default"><br></div><div class="gmail_default" style="font-family:monospace,monospace">This patch was submitted, but not pushed to Linux: </div><div class="gmail_default" style="font-family:monospace,monospace"><div class="gmail_default" style="font-family:Arial,Helvetica,sans-serif"><font face="monospace, monospace"><a href="https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1610776.html">https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1610776.html</a></font><br></div><div class="gmail_default" style="font-family:Arial,Helvetica,sans-serif"><font face="monospace, monospace"><br></font></div><div class="gmail_default" style="font-family:Arial,Helvetica,sans-serif"><font face="monospace, monospace">Meanwhile first version was pushed to the Kernel, So recreate a patch that is relative between the two patches.</font></div><div class="gmail_default" style="font-family:Arial,Helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:Arial,Helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:Arial,Helvetica,sans-serif">More comments inline..</div></div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace"><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Nov 30, 2018 at 2:23 AM Stephen Boyd <<a href="mailto:sboyd@kernel.org" target="_blank">sboyd@kernel.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting <a href="mailto:tali.perry1@gmail.com" target="_blank">tali.perry1@gmail.com</a> (2018-11-11 05:47:12)<br>
> From: Tali Perry <<a href="mailto:tali.perry1@gmail.com" target="_blank">tali.perry1@gmail.com</a>><br>
> <br>
> Nuvoton NPCM7XX Clock Controller<br>
> fix base address and of_clk_get_by_name error handling.<br>
> Also update error messages to be more informative.<br>
> <br>
> In case clk_base allocation is erronoeous the return value is null.<br>
> Also fix handling of of_clk_get_by_name returns an error. <br>
> Print a better error message pointing to the dt-binding documention.<br>
> <br>
> This patch is re-submitted since it was already pushed to main <br>
> (just the diff, without the full driver which is already in <br>
> master branch.)<br>
> <br>
> <br>
> Signed-off-by: Tali Perry <<a href="mailto:tali.perry1@gmail.com" target="_blank">tali.perry1@gmail.com</a>><br>
> Reviewed-by: Rob Herring <<a href="mailto:robh@kernel.org" target="_blank">robh@kernel.org</a>><br>
> Signed-off-by: Wei Yongjun <<a href="mailto:weiyongjun1@huawei.com" target="_blank">weiyongjun1@huawei.com</a>><br>
> <br>
<br>
I don't know what's going on with this patch. Is it something new? Can<br>
you break it up into logical changes and submit them with proper commit<br>
text? The signoff chain is also odd. Can you follow<br>
Documentation/process/submitting-patches.rst?<br>
<br>
> ---<br>
> drivers/clk/clk-npcm7xx.c | 99 +++++++++++++++++++++++++++++++++++------------<br>
> 1 file changed, 75 insertions(+), 24 deletions(-)<br>
> <br>
> diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c<br>
> index 740af90a9508..91a937720f4e 100644<br>
> --- a/drivers/clk/clk-npcm7xx.c<br>
> +++ b/drivers/clk/clk-npcm7xx.c<br>
> @@ -7,17 +7,26 @@<br>
> * Copyright (C) 2018 Nuvoton Technologies <a href="mailto:tali.perry@nuvoton.com" target="_blank">tali.perry@nuvoton.com</a><br>
> */<br>
> <br>
> -#include <linux/module.h><br>
> -#include <linux/clk-provider.h><br>
> -#include <linux/io.h><br>
> -#include <linux/kernel.h><br>
> -#include <linux/of.h><br>
> -#include <linux/of_address.h><br>
> -#include <linux/slab.h><br>
> -#include <linux/err.h><br>
> -#include <linux/bitfield.h><br>
> -<br>
> -#include <dt-bindings/clock/nuvoton,npcm7xx-clock.h><br>
> +<br>
> +<br>
> +<br>
> + #include <linux/module.h><br>
> + #include <linux/clk.h><br>
> + #include <linux/clk-provider.h><br>
> + #include <linux/device.h><br>
> + #include <linux/io.h><br>
> + #include <linux/kernel.h><br>
> + #include <linux/of.h><br>
> + #include <linux/of_device.h><br>
> + #include <linux/of_platform.h><br>
> + #include <linux/of_address.h><br>
> + #include <linux/platform_device.h><br>
> + #include <linux/slab.h><br>
> + #include <linux/err.h><br>
> + #include <linux/rational.h><br>
> + #include <linux/bitfield.h><br>
> + #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h><br>
<br>
Is any of this hunk necessary or relevant to the subject of this patch?<br>
<br>
> +<br>
> <br>
> struct npcm7xx_clk_pll {<br>
> @@ -44,7 +56,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,<br>
> u64 ret;<br>
> <br>
> if (parent_rate == 0) {<br>
> - pr_err("%s: parent rate is zero", __func__);<br>
> + pr_err("%s: parent rate is zero. reg=%x\n", __func__,<br>
> + (u32)(pll->pllcon));<br>
<br>
Maybe you should print clk name instead of register?<br></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">Tali: There are only 4 pllcon registers. I don't mind if it's the address and not the name.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> return 0;<br>
> }<br>
> <br>
> @@ -61,13 +74,12 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct clk_hw *hw,<br>
> return ret;<br>
> }<br>
> <br>
> -static const struct clk_ops npcm7xx_clk_pll_ops = {<br>
> +const struct clk_ops npcm7xx_clk_pll_ops = {<br>
<br>
Why?<br></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">Tali: I will return the static.</div><div class="gmail_default" style="font-family:monospace,monospace"></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> .recalc_rate = npcm7xx_clk_pll_recalc_rate,<br>
> };<br>
> <br>
> -static struct clk_hw *<br>
> -npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,<br>
> - const char *parent_name, unsigned long flags)<br>
> +static struct clk_hw *npcm7xx_clk_register_pll(void __iomem *pllcon,<br>
> + const char *name, const char *parent_name, unsigned long flags)<br>
> {<br>
> struct npcm7xx_clk_pll *pll;<br>
> struct clk_init_data init;<br>
> @@ -78,7 +90,8 @@ npcm7xx_clk_register_pll(void __iomem *pllcon, const char *name,<br>
> if (!pll)<br>
> return ERR_PTR(-ENOMEM);<br>
> <br>
> - pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name);<br>
> + pr_debug("%s reg, reg=0x%x, name=%s, p=%s\n",<br>
> + __func__, (unsigned int)pllcon, name, parent_name);<br>
<br>
What's going on here? Why cast?<br></blockquote><div><span class="gmail_default" style="font-family:monospace,monospace">Tali: I will remove this print all together.</span> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> <br>
> <a href="http://init.name" rel="noreferrer" target="_blank">init.name</a> = name;<br>
> init.ops = &npcm7xx_clk_pll_ops;<br>
> @@ -544,9 +557,11 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)<br>
> void __iomem *clk_base;<br>
> struct resource res;<br>
> struct clk_hw *hw;<br>
> + struct clk *clk;<br>
> int ret;<br>
> int i;<br>
> <br>
> + clk_base = NULL;<br>
<br>
Why?<br></blockquote><div> </div><div><span class="gmail_default" style="font-family:monospace,monospace">Tali: to init it. If DT is badly defined the value will remain NULL.</span></div><div><span class="gmail_default" style="font-family:monospace,monospace"></span> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> ret = of_address_to_resource(clk_np, 0, &res);<br>
> if (ret) {<br>
> pr_err("%s: failed to get resource, ret %d\n", clk_np->name,<br>
> @@ -560,14 +575,44 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)<br>
> <br>
> npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *<br>
> NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);<br>
> - if (!npcm7xx_clk_data)<br>
> +<br>
> + npcm7xx_clk_data->num = 0;<br>
<br>
kzalloc already does that initialization to 0 for you.<br>
<br>
> +<br>
> + if (!npcm7xx_clk_data->hws) {<br>
> + pr_err("Can't alloc npcm7xx_clk_data\n");<br>
<br>
We don't need allocation error messages.<br>
<br>
> goto npcm7xx_init_np_err;<br>
> + }<br>
> <br>
> npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;<br>
> <br>
> for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)<br>
> npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);<br>
> <br>
> + /* Read fixed clocks. These 3 clocks must be defined in DT */<br>
> + clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);<br>
> + if (IS_ERR(clk)) {<br>
> + pr_err("failed to find external REFCLK on device tree, err=%ld\n",<br>
> + PTR_ERR(clk));<br>
> + clk_put(clk);<br>
> + goto npcm7xx_init_fail_no_clk_on_dt;<br>
> + }<br>
> +<br>
> + clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);<br>
> + if (IS_ERR(clk)) {<br>
> + pr_err("failed to find external SYSBYPCK on device tree, err=%ld\n",<br>
> + PTR_ERR(clk));<br>
> + clk_put(clk);<br>
> + goto npcm7xx_init_fail_no_clk_on_dt;<br>
> + }<br>
> +<br>
> + clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);<br>
> + if (IS_ERR(clk)) {<br>
> + pr_err("failed to find external MCBYPCK on device tree, err=%ld\n",<br>
> + PTR_ERR(clk));<br>
> + clk_put(clk);<br>
> + goto npcm7xx_init_fail_no_clk_on_dt;<br>
> + }<br>
<br>
I assume the DTS is not shipped?<br></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">Tali: DTS for npcm is shipped in a separate patchset.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> /* Register plls */<br>
> for (i = 0; i < ARRAY_SIZE(npcm7xx_plls); i++) {<br>
> const struct npcm7xx_clk_pll_data *pll_data = &npcm7xx_plls[i];<br>
> @@ -584,16 +629,16 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)<br>
> }<br>
> <br>
> /* Register fixed dividers */<br>
> - hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,<br>
> + clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL1_DIV2,<br>
<br>
This is going backwards. Please use clk_hw APIs.<br></blockquote><div><span class="gmail_default" style="font-family:monospace,monospace">Tali: Will fix.</span> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> NPCM7XX_CLK_S_PLL1, 0, 1, 2);<br>
> - if (IS_ERR(hw)) {<br>
> + if (IS_ERR(clk)) {<br>
> pr_err("npcm7xx_clk: Can't register fixed div\n");<br>
> goto npcm7xx_init_fail;<br>
> }<br>
> <br>
> - hw = clk_hw_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,<br>
> + clk = clk_register_fixed_factor(NULL, NPCM7XX_CLK_S_PLL2_DIV2,<br>
> NPCM7XX_CLK_S_PLL2, 0, 1, 2);<br>
> - if (IS_ERR(hw)) {<br>
> + if (IS_ERR(clk)) {<br>
> pr_err("npcm7xx_clk: Can't register div2\n");<br>
> goto npcm7xx_init_fail;<br>
> }<br>
> @@ -646,11 +691,17 @@ static void __init npcm7xx_clk_init(struct device_node *clk_np)<br>
> <br>
> return;<br>
> <br>
> +npcm7xx_init_fail_no_clk_on_dt:<br>
> + pr_err("see Documentation/devicetree/bindings/clock/");<br>
> + pr_err("nuvoton,npcm750-clk.txt for details\n");<br>
> npcm7xx_init_fail:<br>
> - kfree(npcm7xx_clk_data->hws);<br>
> + if (npcm7xx_clk_data->num)<br>
> + kfree(npcm7xx_clk_data->hws);<br>
> npcm7xx_init_np_err:<br>
> - iounmap(clk_base);<br>
> + if (clk_base != NULL)<br>
> + iounmap(clk_base);<br>
> npcm7xx_init_error:<br>
> of_node_put(clk_np);<br>
> + pr_err("clk setup fail\n");<br>
<br>
Is this hunk of changes related to the subject of this patch? I don't<br>
think it is, so it just leads to more confusion.<br></blockquote><div><br></div><div class="gmail_default" style="font-family:monospace,monospace">Tali: I added a goto option for the case that the DTS doesn't have the reference clocks this module needs.</div><div class="gmail_default" style="font-family:monospace,monospace"><br></div><div class="gmail_default" style="font-family:monospace,monospace"></div></div></div>