* Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller
[not found] ` <20230309194402.119562-2-tmaimon77@gmail.com>
@ 2023-03-20 19:50 ` Stephen Boyd
2023-03-31 18:07 ` Tomer Maimon
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2023-03-20 19:50 UTC (permalink / raw)
To: Tomer Maimon, avifishman70, benjaminfair, joel, mturquette,
tali.perry1, venture, yuenn
Cc: openbmc, Tomer Maimon, linux-clk, linux-kernel
Quoting Tomer Maimon (2023-03-09 11:44:02)
> diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> new file mode 100644
> index 000000000000..67058f121251
> --- /dev/null
> +++ b/drivers/clk/clk-npcm8xx.c
> @@ -0,0 +1,561 @@
[...]
> +
> +struct npcm8xx_pll_data {
> + const char *name;
> + struct clk_parent_data parent;
> + unsigned int reg;
> + unsigned long flags;
> +};
> +
> +struct npcm8xx_clk_div_data {
> + u32 reg;
> + u8 shift;
> + u8 width;
> + const char *name;
> + const struct clk_parent_data parent_data;
> + u8 clk_divider_flags;
> + unsigned long flags;
> + int onecell_idx;
> +};
> +
> +struct npcm8xx_clk_mux_data {
> + u8 shift;
> + u32 mask;
> + const u32 *table;
> + const char *name;
> + const struct clk_parent_data *parent_data;
> + u8 num_parents;
> + unsigned long flags;
> +};
> +
[...]
> +
> +static struct npcm8xx_pll_data npcm8xx_pll_clks[] = {
Can this be const?
> + { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 },
> + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 },
> + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 },
> + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 },
> +};
> +
> +static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 };
> +static const struct clk_parent_data cpuck_mux_parents[] = {
> + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
You should only have .fw_name or .index when introducing new drivers.
The .name field is for existing drivers that want to migrate to
clk_parent_data.
> + { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 },
> + { .name = NPCM8XX_CLK_S_REFCLK },
Note, this line says to use '.index = 0', and .name will be ignored.
Maybe just use the index for everything? That makes it simpler and
potentially faster because we don't have to do string comparisons
anywhere.
> + { .fw_name = NPCM8XX_CLK_S_PLL2, .name = NPCM8XX_CLK_S_PLL2 }
> +};
> +
> +static const u32 pixcksel_mux_table[] = { 0, 2 };
> +static const struct clk_parent_data pixcksel_mux_parents[] = {
> + { .fw_name = NPCM8XX_CLK_S_PLL_GFX, .name = NPCM8XX_CLK_S_PLL_GFX },
> + { .name = NPCM8XX_CLK_S_REFCLK }
> +};
> +
[...]
> +
> +static unsigned long npcm8xx_clk_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct npcm8xx_clk_pll *pll = to_npcm8xx_clk_pll(hw);
> + unsigned long fbdv, indv, otdv1, otdv2;
> + unsigned int val;
> + u64 ret;
> +
> + if (parent_rate == 0) {
> + pr_debug("%s: parent rate is zero\n", __func__);
> + return 0;
> + }
> +
> + val = readl_relaxed(pll->pllcon + pll->reg);
Is pll->reg ever set?
> +
> + indv = FIELD_GET(PLLCON_INDV, val);
> + fbdv = FIELD_GET(PLLCON_FBDV, val);
> + otdv1 = FIELD_GET(PLLCON_OTDV1, val);
> + otdv2 = FIELD_GET(PLLCON_OTDV2, val);
> +
> + ret = (u64)parent_rate * fbdv;
> + do_div(ret, indv * otdv1 * otdv2);
> +
> + return ret;
> +}
> +
> +static const struct clk_ops npcm8xx_clk_pll_ops = {
> + .recalc_rate = npcm8xx_clk_pll_recalc_rate,
> +};
> +
> +static struct clk_hw *
> +npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
> + const char *name, const struct clk_parent_data *parent,
> + unsigned long flags)
> +{
> + struct npcm8xx_clk_pll *pll;
> + struct clk_init_data init = {};
> + int ret;
> +
> + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> + if (!pll)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + init.ops = &npcm8xx_clk_pll_ops;
> + init.parent_data = parent;
> + init.num_parents = 1;
> + init.flags = flags;
> +
> + pll->pllcon = pllcon;
> + pll->hw.init = &init;
> +
> + ret = devm_clk_hw_register(dev, &pll->hw);
> + if (ret) {
> + kfree(pll);
> + return ERR_PTR(ret);
> + }
> +
> + return &pll->hw;
> +}
> +
> +static DEFINE_SPINLOCK(npcm8xx_clk_lock);
> +
> +static int npcm8xx_clk_probe(struct platform_device *pdev)
> +{
> + struct clk_hw_onecell_data *npcm8xx_clk_data;
> + struct device *dev = &pdev->dev;
> + void __iomem *clk_base;
> + struct resource *res;
> + struct clk_hw *hw;
> + unsigned int i;
> + int err;
> +
> + npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> + NPCM8XX_NUM_CLOCKS),
> + GFP_KERNEL);
> + if (!npcm8xx_clk_data)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + clk_base = devm_ioremap(dev, res->start, resource_size(res));
Can you use devm_platform_ioremap_resource() instead?
> + if (!clk_base) {
Then this is checked for error pointer.
> + dev_err(&pdev->dev, "Failed to remap I/O memory\n");
And no error message.
> + return -ENOMEM;
> + }
> +
> + npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
> +
[....]
> + if (IS_ERR(hw)) {
> + dev_err(dev, "npcm8xx_clk: Can't register axi div2\n");
> + return PTR_ERR(hw);
> + }
> + npcm8xx_clk_data->hws[NPCM8XX_CLK_AXI] = hw;
> +
> + hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB,
> + NPCM8XX_CLK_S_AXI, 0, 1, 2);
> + if (IS_ERR(hw)) {
> + dev_err(dev, "npcm8xx_clk: Can't register atb div2\n");
> + return PTR_ERR(hw);
> + }
> + npcm8xx_clk_data->hws[NPCM8XX_CLK_ATB] = hw;
> +
> + /* Register clock dividers specified in npcm8xx_divs */
> + for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) {
> + const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i];
> +
> + hw = clk_hw_register_divider_parent_data(dev, div_data->name,
Do we have a devm_ variant of this function? If not, can you add it?
> + &div_data->parent_data,
> + div_data->flags,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller
2023-03-20 19:50 ` [PATCH v15 1/1] clk: npcm8xx: add clock controller Stephen Boyd
@ 2023-03-31 18:07 ` Tomer Maimon
2023-04-05 19:09 ` Stephen Boyd
0 siblings, 1 reply; 4+ messages in thread
From: Tomer Maimon @ 2023-03-31 18:07 UTC (permalink / raw)
To: Stephen Boyd
Cc: benjaminfair, avifishman70, venture, mturquette, linux-clk,
linux-kernel, tali.perry1, joel, openbmc
HI Stephen,
Thanks for your comments
On Mon, 20 Mar 2023 at 21:50, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Tomer Maimon (2023-03-09 11:44:02)
> > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > new file mode 100644
> > index 000000000000..67058f121251
> > --- /dev/null
> > +++ b/drivers/clk/clk-npcm8xx.c
> > @@ -0,0 +1,561 @@
> [...]
> > +
> > +struct npcm8xx_pll_data {
> > + const char *name;
> > + struct clk_parent_data parent;
> > + unsigned int reg;
> > + unsigned long flags;
> > +};
> > +
> > +struct npcm8xx_clk_div_data {
> > + u32 reg;
> > + u8 shift;
> > + u8 width;
> > + const char *name;
> > + const struct clk_parent_data parent_data;
> > + u8 clk_divider_flags;
> > + unsigned long flags;
> > + int onecell_idx;
> > +};
> > +
> > +struct npcm8xx_clk_mux_data {
> > + u8 shift;
> > + u32 mask;
> > + const u32 *table;
> > + const char *name;
> > + const struct clk_parent_data *parent_data;
> > + u8 num_parents;
> > + unsigned long flags;
> > +};
> > +
> [...]
> > +
> > +static struct npcm8xx_pll_data npcm8xx_pll_clks[] = {
>
> Can this be const?
Will add
>
> > + { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 },
> > + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 },
> > + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 },
> > + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 },
> > +};
> > +
> > +static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 };
> > +static const struct clk_parent_data cpuck_mux_parents[] = {
> > + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
>
> You should only have .fw_name or .index when introducing new drivers.
> The .name field is for existing drivers that want to migrate to
> clk_parent_data.
I thought using .name was done when the clock defines in the DT, like
the ref clock.
If the other clocks are not defined both .fw_name and .name the clocks
are not registered properly.
>
> > + { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 },
> > + { .name = NPCM8XX_CLK_S_REFCLK },
>
> Note, this line says to use '.index = 0', and .name will be ignored.
> Maybe just use the index for everything? That makes it simpler and
> potentially faster because we don't have to do string comparisons
> anywhere.
Should the clk_parent_data mux use only .index? if yes how should the
clock tree have a connection between the parent's clock and the mux
for example:
for example, how should the driver connect between
NPCM8XX_CLK_S_PLL1_DIV2 and the index number in the clk_parent_data?
>
> > + { .fw_name = NPCM8XX_CLK_S_PLL2, .name = NPCM8XX_CLK_S_PLL2 }
> > +};
> > +
> > +static const u32 pixcksel_mux_table[] = { 0, 2 };
> > +static const struct clk_parent_data pixcksel_mux_parents[] = {
> > + { .fw_name = NPCM8XX_CLK_S_PLL_GFX, .name = NPCM8XX_CLK_S_PLL_GFX },
> > + { .name = NPCM8XX_CLK_S_REFCLK }
> > +};
> > +
> [...]
> > +
> > +static unsigned long npcm8xx_clk_pll_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + struct npcm8xx_clk_pll *pll = to_npcm8xx_clk_pll(hw);
> > + unsigned long fbdv, indv, otdv1, otdv2;
> > + unsigned int val;
> > + u64 ret;
> > +
> > + if (parent_rate == 0) {
> > + pr_debug("%s: parent rate is zero\n", __func__);
> > + return 0;
> > + }
> > +
> > + val = readl_relaxed(pll->pllcon + pll->reg);
>
> Is pll->reg ever set?
Will remove.
>
> > +
> > + indv = FIELD_GET(PLLCON_INDV, val);
> > + fbdv = FIELD_GET(PLLCON_FBDV, val);
> > + otdv1 = FIELD_GET(PLLCON_OTDV1, val);
> > + otdv2 = FIELD_GET(PLLCON_OTDV2, val);
> > +
> > + ret = (u64)parent_rate * fbdv;
> > + do_div(ret, indv * otdv1 * otdv2);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct clk_ops npcm8xx_clk_pll_ops = {
> > + .recalc_rate = npcm8xx_clk_pll_recalc_rate,
> > +};
> > +
> > +static struct clk_hw *
> > +npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
> > + const char *name, const struct clk_parent_data *parent,
> > + unsigned long flags)
> > +{
> > + struct npcm8xx_clk_pll *pll;
> > + struct clk_init_data init = {};
> > + int ret;
> > +
> > + pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> > + if (!pll)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + init.name = name;
> > + init.ops = &npcm8xx_clk_pll_ops;
> > + init.parent_data = parent;
> > + init.num_parents = 1;
> > + init.flags = flags;
> > +
> > + pll->pllcon = pllcon;
> > + pll->hw.init = &init;
> > +
> > + ret = devm_clk_hw_register(dev, &pll->hw);
> > + if (ret) {
> > + kfree(pll);
> > + return ERR_PTR(ret);
> > + }
> > +
> > + return &pll->hw;
> > +}
> > +
> > +static DEFINE_SPINLOCK(npcm8xx_clk_lock);
> > +
> > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > +{
> > + struct clk_hw_onecell_data *npcm8xx_clk_data;
> > + struct device *dev = &pdev->dev;
> > + void __iomem *clk_base;
> > + struct resource *res;
> > + struct clk_hw *hw;
> > + unsigned int i;
> > + int err;
> > +
> > + npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> > + NPCM8XX_NUM_CLOCKS),
> > + GFP_KERNEL);
> > + if (!npcm8xx_clk_data)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + clk_base = devm_ioremap(dev, res->start, resource_size(res));
>
> Can you use devm_platform_ioremap_resource() instead?
We should use devm_ioremap since the clock register is used for the
reset driver as well.
>
> > + if (!clk_base) {
>
> Then this is checked for error pointer.
>
> > + dev_err(&pdev->dev, "Failed to remap I/O memory\n");
>
> And no error message.
>
> > + return -ENOMEM;
> > + }
> > +
> > + npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
> > +
> [....]
> > + if (IS_ERR(hw)) {
> > + dev_err(dev, "npcm8xx_clk: Can't register axi div2\n");
> > + return PTR_ERR(hw);
> > + }
> > + npcm8xx_clk_data->hws[NPCM8XX_CLK_AXI] = hw;
> > +
> > + hw = devm_clk_hw_register_fixed_factor(dev, NPCM8XX_CLK_S_ATB,
> > + NPCM8XX_CLK_S_AXI, 0, 1, 2);
> > + if (IS_ERR(hw)) {
> > + dev_err(dev, "npcm8xx_clk: Can't register atb div2\n");
> > + return PTR_ERR(hw);
> > + }
> > + npcm8xx_clk_data->hws[NPCM8XX_CLK_ATB] = hw;
> > +
> > + /* Register clock dividers specified in npcm8xx_divs */
> > + for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) {
> > + const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i];
> > +
> > + hw = clk_hw_register_divider_parent_data(dev, div_data->name,
>
> Do we have a devm_ variant of this function? If not, can you add it?
I will try to do it.
>
> > + &div_data->parent_data,
> > + div_data->flags,
Thanks,
Tomer
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller
2023-03-31 18:07 ` Tomer Maimon
@ 2023-04-05 19:09 ` Stephen Boyd
2023-05-21 15:51 ` Tomer Maimon
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2023-04-05 19:09 UTC (permalink / raw)
To: Tomer Maimon
Cc: benjaminfair, avifishman70, venture, mturquette, linux-clk,
linux-kernel, tali.perry1, joel, openbmc
Quoting Tomer Maimon (2023-03-31 11:07:19)
> On Mon, 20 Mar 2023 at 21:50, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2023-03-09 11:44:02)
> > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > new file mode 100644
> > > index 000000000000..67058f121251
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-npcm8xx.c
> > > + { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 },
> > > + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 },
> > > + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 },
> > > + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 },
> > > +};
> > > +
> > > +static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 };
> > > +static const struct clk_parent_data cpuck_mux_parents[] = {
> > > + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
> >
> > You should only have .fw_name or .index when introducing new drivers.
> > The .name field is for existing drivers that want to migrate to
> > clk_parent_data.
> I thought using .name was done when the clock defines in the DT, like
> the ref clock.
> If the other clocks are not defined both .fw_name and .name the clocks
> are not registered properly.
Are you saying that having .name fixes it?
> >
> > > + { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 },
> > > + { .name = NPCM8XX_CLK_S_REFCLK },
> >
> > Note, this line says to use '.index = 0', and .name will be ignored.
> > Maybe just use the index for everything? That makes it simpler and
> > potentially faster because we don't have to do string comparisons
> > anywhere.
> Should the clk_parent_data mux use only .index? if yes how should the
> clock tree have a connection between the parent's clock and the mux
> for example:
> for example, how should the driver connect between
> NPCM8XX_CLK_S_PLL1_DIV2 and the index number in the clk_parent_data?
It's not required, but it makes things simpler to only use .index or
direct clk_hw pointers (.hw). I'm working on a clk documentation
overhaul series right now, about 4 years later than I should have done
it. It will cover this.
The .index field corresponds to the cell index in your devicetree
'clocks' property of the clk provider (the node with #clock-cells
property). If the clk is internal, just use a .hw member and point to it
directly. Don't consume your own clks in DT. If NPCM8XX_CLK_S_PLL1_DIV2
is a clk provided/registered by this device then it should be pointed to
directly with the clk_hw pointer. If NPCM8XX_CLK_S_PLL1_DIV2 is an
external clk that is consumed via the 'clocks' property in DT, then it
should be specified as a parent via the .index member.
> > > +
> > > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > > +{
> > > + struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > + struct device *dev = &pdev->dev;
> > > + void __iomem *clk_base;
> > > + struct resource *res;
> > > + struct clk_hw *hw;
> > > + unsigned int i;
> > > + int err;
> > > +
> > > + npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> > > + NPCM8XX_NUM_CLOCKS),
> > > + GFP_KERNEL);
> > > + if (!npcm8xx_clk_data)
> > > + return -ENOMEM;
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + clk_base = devm_ioremap(dev, res->start, resource_size(res));
> >
> > Can you use devm_platform_ioremap_resource() instead?
> We should use devm_ioremap since the clock register is used for the
> reset driver as well.
Are the clk and reset drivers sharing the register range? If so, please
use auxiliary bus to register the reset driver, and map the register
region once in the driver that registers the auxiliary device. Pass the
iomem pointer to the auxiliary device.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v15 1/1] clk: npcm8xx: add clock controller
2023-04-05 19:09 ` Stephen Boyd
@ 2023-05-21 15:51 ` Tomer Maimon
0 siblings, 0 replies; 4+ messages in thread
From: Tomer Maimon @ 2023-05-21 15:51 UTC (permalink / raw)
To: Stephen Boyd
Cc: benjaminfair, avifishman70, venture, mturquette, linux-clk,
linux-kernel, tali.perry1, joel, openbmc
Hi Stephen,
Sorry for the late reply and thanks a lot for your clarifications.
I have sent today V16 with some changes according to your comments.
On Wed, 5 Apr 2023 at 22:09, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Tomer Maimon (2023-03-31 11:07:19)
> > On Mon, 20 Mar 2023 at 21:50, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Tomer Maimon (2023-03-09 11:44:02)
> > > > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > > > new file mode 100644
> > > > index 000000000000..67058f121251
> > > > --- /dev/null
> > > > +++ b/drivers/clk/clk-npcm8xx.c
> > > > + { NPCM8XX_CLK_S_PLL0, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON0, 0 },
> > > > + { NPCM8XX_CLK_S_PLL1, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON1, 0 },
> > > > + { NPCM8XX_CLK_S_PLL2, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCON2, 0 },
> > > > + { NPCM8XX_CLK_S_PLL_GFX, { .name = NPCM8XX_CLK_S_REFCLK }, NPCM8XX_PLLCONG, 0 },
> > > > +};
> > > > +
> > > > +static const u32 cpuck_mux_table[] = { 0, 1, 2, 7 };
> > > > +static const struct clk_parent_data cpuck_mux_parents[] = {
> > > > + { .fw_name = NPCM8XX_CLK_S_PLL0, .name = NPCM8XX_CLK_S_PLL0 },
> > >
> > > You should only have .fw_name or .index when introducing new drivers.
> > > The .name field is for existing drivers that want to migrate to
> > > clk_parent_data.
> > I thought using .name was done when the clock defines in the DT, like
> > the ref clock.
> > If the other clocks are not defined both .fw_name and .name the clocks
> > are not registered properly.
>
> Are you saying that having .name fixes it?
>
> > >
> > > > + { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 },
> > > > + { .name = NPCM8XX_CLK_S_REFCLK },
> > >
> > > Note, this line says to use '.index = 0', and .name will be ignored.
> > > Maybe just use the index for everything? That makes it simpler and
> > > potentially faster because we don't have to do string comparisons
> > > anywhere.
> > Should the clk_parent_data mux use only .index? if yes how should the
> > clock tree have a connection between the parent's clock and the mux
> > for example:
> > for example, how should the driver connect between
> > NPCM8XX_CLK_S_PLL1_DIV2 and the index number in the clk_parent_data?
>
> It's not required, but it makes things simpler to only use .index or
> direct clk_hw pointers (.hw). I'm working on a clk documentation
> overhaul series right now, about 4 years later than I should have done
> it. It will cover this.
>
> The .index field corresponds to the cell index in your devicetree
> 'clocks' property of the clk provider (the node with #clock-cells
> property). If the clk is internal, just use a .hw member and point to it
> directly. Don't consume your own clks in DT. If NPCM8XX_CLK_S_PLL1_DIV2
> is a clk provided/registered by this device then it should be pointed to
> directly with the clk_hw pointer. If NPCM8XX_CLK_S_PLL1_DIV2 is an
> external clk that is consumed via the 'clocks' property in DT, then it
> should be specified as a parent via the .index member.
>
> > > > +
> > > > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > > + struct device *dev = &pdev->dev;
> > > > + void __iomem *clk_base;
> > > > + struct resource *res;
> > > > + struct clk_hw *hw;
> > > > + unsigned int i;
> > > > + int err;
> > > > +
> > > > + npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> > > > + NPCM8XX_NUM_CLOCKS),
> > > > + GFP_KERNEL);
> > > > + if (!npcm8xx_clk_data)
> > > > + return -ENOMEM;
> > > > +
> > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > + clk_base = devm_ioremap(dev, res->start, resource_size(res));
> > >
> > > Can you use devm_platform_ioremap_resource() instead?
> > We should use devm_ioremap since the clock register is used for the
> > reset driver as well.
>
We using the same ioremap sequence in npcm7xx clock driver since the
clock and reset share the same register region.
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
clk_base = devm_ioremap(dev, res->start, resource_size(res));
https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/clk/clk-npcm7xx.c#L413
Why we cannot use it in the NPCM8XX clock driver?
> Are the clk and reset drivers sharing the register range? If so, please
> use auxiliary bus to register the reset driver, and map the register
> region once in the driver that registers the auxiliary device. Pass the
> iomem pointer to the auxiliary device.
Sorry, but I didn't understand what you mean by using the auxiliary
bus to register the reset driver do you have an example of how it
should be done?
Thanks,
Tomer
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-21 15:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230309194402.119562-1-tmaimon77@gmail.com>
[not found] ` <20230309194402.119562-2-tmaimon77@gmail.com>
2023-03-20 19:50 ` [PATCH v15 1/1] clk: npcm8xx: add clock controller Stephen Boyd
2023-03-31 18:07 ` Tomer Maimon
2023-04-05 19:09 ` Stephen Boyd
2023-05-21 15:51 ` Tomer Maimon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).