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