[v6,1/3] clk: x86: Add Atom PMC platform clocks

Commit Message

tip-bot for Irina Tirdea Dec. 9, 2016, 6:01 p.m. UTC

The BayTrail and CherryTrail platforms provide platform clocks
through their Power Management Controller (PMC).

The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
are available for general system use, where appropriate, and each
have Control & Frequency register fields associated with them.

Signed-off-by: Irina Tirdea <[e-mail]>
Signed-off-by: Pierre-Louis Bossart <[e-mail]>
---
 drivers/clk/x86/Makefile                      |   3 +
 drivers/clk/x86/clk-byt-plt.c                 | 380 ++++++++++++++++++++++++++
 include/linux/platform_data/x86/clk-byt-plt.h |  31 +++
 3 files changed, 414 insertions(+)
 create mode 100644 drivers/clk/x86/clk-byt-plt.c
 create mode 100644 include/linux/platform_data/x86/clk-byt-plt.h

Comments

Andy Shevchenko Dec. 12, 2016, 11:39 p.m. UTC | #1

On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <[e-mail]> wrote:

Thanks for an update I will comment all the patches.
Here we start.

> The BayTrail and CherryTrail platforms provide platform clocks
> through their Power Management Controller (PMC).
>
> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
> are available for general system use, where appropriate, and each
> have Control & Frequency register fields associated with them.
>
> Signed-off-by: Irina Tirdea <[e-mail]>
> Signed-off-by: Pierre-Louis Bossart <[e-mail]>

Who is the actual author? SoB I guess should be either the author, or
1st, 2nd, ..., last one who is submitter.

> --- a/drivers/clk/x86/Makefile
> +++ b/drivers/clk/x86/Makefile
> @@ -1,2 +1,5 @@
>  clk-x86-lpss-objs              := clk-lpt.o
>  obj-$(CONFIG_X86_INTEL_LPSS)   += clk-x86-lpss.o

> +ifeq ($(CONFIG_COMMON_CLK), y)

Hmm... I think (I didn't check) we don't go here otherwise.

> +obj-$(CONFIG_PMC_ATOM)         += clk-byt-plt.o

I'm pretty sure X86_INTEL_LPSS almost replicates what you need (it
also includes Haswell support, but it doesn't matter here).

Can we unify them or is it a bad idea?

Otherwise I would propose to rename module to be something like
clk-x86-pmc.o which follows above pattern: LPSS as provider, PMC as
provider and so on.

Maybe
clk-x86-pmc-objs              := clk-pmc-atom.o
...

By the way lpt is a not good chosen abbreviation for Lynxpoint. I even
had a patch to get rid of this file completely.

> +endif
> diff --git a/drivers/clk/x86/clk-byt-plt.c b/drivers/clk/x86/clk-byt-plt.c
> new file mode 100644
> index 0000000..2303e0d
> --- /dev/null
> +++ b/drivers/clk/x86/clk-byt-plt.c

> @@ -0,0 +1,380 @@
> +/*
> + * Intel Atom platform clocks driver for BayTrail and CherryTrail SoC.

SoCs.

> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Irina Tirdea <[e-mail]>

Be consistent with SoB lines above.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/clkdev.h>

> +#include <linux/platform_data/x86/clk-byt-plt.h>

Is it indeed platform data? I would not create platform_data/x86
without strong argument.
Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
which is old enough and was basically first try of clk stuff on x86)

> +
> +#define PLT_CLK_NAME_BASE      "pmc_plt_clk_"

> +#define PLT_CLK_DRIVER_NAME    "clk-byt-plt"

By default you may use build name of the module, but if you want to
stick with something choose the name I mentioned above like
clk-pmc-atom.

>
> +#define PMC_CLK_CTL_SIZE       4
> +#define PMC_CLK_NUM            6
> +#define PMC_MASK_CLK_CTL       GENMASK(1, 0)
> +#define PMC_MASK_CLK_FREQ      BIT(2)
> +#define PMC_CLK_CTL_GATED_ON_D3        0x0
> +#define PMC_CLK_CTL_FORCE_ON   0x1
> +#define PMC_CLK_CTL_FORCE_OFF  0x2
> +#define PMC_CLK_CTL_RESERVED   0x3

> +#define PMC_CLK_FREQ_XTAL      0x0     /* 25 MHz */
> +#define PMC_CLK_FREQ_PLL       0x4     /* 19.2 MHz */

Looks like (0 << 2) and (1 << 2). I would put that way to show that
it's bitwise value.

> +
> +struct clk_plt_fixed {

Yeah, rename names accordingly.

> +       struct clk_hw *clk;
> +       struct clk_lookup *lookup;
> +};
> +
> +struct clk_plt {
> +       struct clk_hw hw;
> +       void __iomem *reg;
> +       struct clk_lookup *lookup;

> +       spinlock_t lock;

Would be nice to have a comment what is/are protected by it.

> +};
> +
> +#define to_clk_plt(_hw) container_of(_hw, struct clk_plt, hw)
> +
> +struct clk_plt_data {
> +       struct clk_plt_fixed **parents;
> +       u8 nparents;
> +       struct clk_plt *clks[PMC_CLK_NUM];
> +};
> +
> +static inline int plt_reg_to_parent(int reg)
> +{
> +       switch (reg & PMC_MASK_CLK_FREQ) {

+ default: (see below) ?

> +       case PMC_CLK_FREQ_XTAL:
> +               return 0;       /* index 0 in parents[] */
> +       case PMC_CLK_FREQ_PLL:
> +               return 1;       /* index 1 in parents[] */
> +       }
> +

> +       return 0;

default: ?

> +}
> +
> +static inline int plt_parent_to_reg(int index)
> +{
> +       switch (index) {
> +       case 0: /* index 0 in parents[] */
> +               return PMC_CLK_FREQ_XTAL;
> +       case 1: /* index 0 in parents[] */
> +               return PMC_CLK_FREQ_PLL;
> +       }
> +
> +       return PMC_CLK_FREQ_XTAL;

Ditto.

> +}
> +
> +static inline int plt_reg_to_enabled(int reg)
> +{
> +       switch (reg & PMC_MASK_CLK_CTL) {
> +       case PMC_CLK_CTL_GATED_ON_D3:
> +       case PMC_CLK_CTL_FORCE_ON:
> +               return 1;       /* enabled */
> +       case PMC_CLK_CTL_FORCE_OFF:
> +       case PMC_CLK_CTL_RESERVED:
> +       default:
> +               return 0;       /* disabled */
> +       }
> +}
> +
> +static void plt_clk_reg_update(struct clk_plt *clk, u32 mask, u32 val)
> +{
> +       u32 orig, tmp;
> +       unsigned long flags = 0;

Redundant assignment.

> +
> +       spin_lock_irqsave(&clk->lock, flags);
> +
> +       orig = readl(clk->reg);
> +
> +       tmp = orig & ~mask;
> +       tmp |= val & mask;
> +

> +       if (tmp != orig)

Hmm...Is here any benefit? Do we do this 1000s times per ...s? OTOH
readability a bit better w/o it.

> +               writel(tmp, clk->reg);
> +
> +       spin_unlock_irqrestore(&clk->lock, flags);
> +}
> +
> +static int plt_clk_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +
> +       plt_clk_reg_update(clk, PMC_MASK_CLK_FREQ, plt_parent_to_reg(index));
> +
> +       return 0;
> +}
> +
> +static u8 plt_clk_get_parent(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +       u32 value;
> +
> +       value = readl(clk->reg);
> +
> +       return plt_reg_to_parent(value);
> +}
> +
> +static int plt_clk_enable(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +
> +       plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_ON);
> +
> +       return 0;
> +}
> +
> +static void plt_clk_disable(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +
> +       plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_OFF);
> +}
> +
> +static int plt_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +       u32 value;
> +
> +       value = readl(clk->reg);
> +
> +       return plt_reg_to_enabled(value);
> +}
> +
> +static const struct clk_ops plt_clk_ops = {
> +       .enable = plt_clk_enable,
> +       .disable = plt_clk_disable,
> +       .is_enabled = plt_clk_is_enabled,
> +       .get_parent = plt_clk_get_parent,
> +       .set_parent = plt_clk_set_parent,
> +       .determine_rate = __clk_mux_determine_rate,
> +};
> +
> +static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,

I don't see how pdev is involved, perhaps just struct device *dev here.

> +                                       void __iomem *base,
> +                                       const char **parent_names,
> +                                       int num_parents)
> +{
> +       struct clk_plt *pclk;
> +       struct clk_init_data init;
> +       int ret;
> +
> +       pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
> +       if (!pclk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name =  kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id);

devm_kasprintf()

> +       init.ops = &plt_clk_ops;
> +       init.flags = 0;
> +       init.parent_names = parent_names;
> +       init.num_parents = num_parents;
> +
> +       pclk->hw.init = &init;
> +       pclk->reg = base + id * PMC_CLK_CTL_SIZE;
> +       spin_lock_init(&pclk->lock);
> +
> +       ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
> +       if (ret)
> +               goto err_free_init;
> +
> +       pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL);
> +       if (!pclk->lookup) {
> +               ret = -ENOMEM;
> +               goto err_free_init;
> +       }
> +

> +       kfree(init.name);

devm_kfree();

> +
> +       return pclk;

> +
> +err_free_init:
> +       kfree(init.name);
> +       return ERR_PTR(ret);

Might be redundant, see above.

> +}
> +
> +static void plt_clk_unregister(struct clk_plt *pclk)
> +{
> +       clkdev_drop(pclk->lookup);
> +}
> +
> +static struct clk_plt_fixed *plt_clk_register_fixed_rate(struct platform_device *pdev,
> +                                                const char *name,
> +                                                const char *parent_name,
> +                                                unsigned long fixed_rate)
> +{
> +       struct clk_plt_fixed *pclk;
> +       int ret = 0;

Useless assignment.

> +
> +       pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
> +       if (!pclk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pclk->clk = clk_hw_register_fixed_rate(&pdev->dev, name, parent_name,
> +                                              0, fixed_rate);
> +       if (IS_ERR(pclk->clk))
> +               return ERR_CAST(pclk->clk);
> +
> +       pclk->lookup = clkdev_hw_create(pclk->clk, name, NULL);
> +       if (!pclk->lookup) {
> +               ret = -ENOMEM;
> +               goto err_clk_unregister;
> +       }
> +
> +       return pclk;
> +
> +err_clk_unregister:
> +       clk_hw_unregister_fixed_rate(pclk->clk);
> +       return ERR_PTR(ret);
> +}
> +
> +static void plt_clk_unregister_fixed_rate(struct clk_plt_fixed *pclk)
> +{
> +       clkdev_drop(pclk->lookup);
> +       clk_hw_unregister_fixed_rate(pclk->clk);
> +}
> +
> +static const char **plt_clk_register_parents(struct platform_device *pdev,
> +                                            struct clk_plt_data *data,
> +                                            const struct pmc_clk *clks)
> +{
> +       const char **parent_names;
> +       int i, err;
> +
> +       data->nparents = 0;
> +       while (clks[data->nparents].name)
> +               data->nparents++;
> +
> +       data->parents = devm_kcalloc(&pdev->dev, data->nparents,
> +                                    sizeof(*data->parents), GFP_KERNEL);
> +       if (!data->parents) {
> +               err = -ENOMEM;
> +               goto err_out;
> +       }
> +
> +       parent_names = kcalloc(data->nparents, sizeof(*parent_names),
> +                              GFP_KERNEL);
> +       if (!parent_names) {
> +               err = -ENOMEM;
> +               goto err_out;
> +       }
> +
> +       for (i = 0; i < data->nparents; i++) {
> +               data->parents[i] =
> +                       plt_clk_register_fixed_rate(pdev, clks[i].name,
> +                                                   clks[i].parent_name,
> +                                                   clks[i].freq);
> +               if (IS_ERR(data->parents[i])) {
> +                       err = PTR_ERR(data->parents[i]);
> +                       goto err_unreg;
> +               }
> +               parent_names[i] = kstrdup_const(clks[i].name, GFP_KERNEL);
> +       }
> +
> +       return parent_names;
> +
> +err_unreg:
> +       for (i--; i >= 0; i--) {

while (i--) {
}

> +               plt_clk_unregister_fixed_rate(data->parents[i]);
> +               kfree_const(parent_names[i]);
> +       }
> +       kfree(parent_names);

> +err_out:
> +       data->nparents = 0;

You will not need this if you define local variable for nparents and
assign data->nparents at last in the function.

> +       return ERR_PTR(err);
> +}
> +
> +static void plt_clk_unregister_parents(struct clk_plt_data *data)
> +{
> +       int i;
> +

> +       for (i = 0; i < data->nparents; i++)
> +               plt_clk_unregister_fixed_rate(data->parents[i]);



> +}
> +
> +static int plt_clk_probe(struct platform_device *pdev)
> +{
> +       struct clk_plt_data *data;
> +       int i, err;
> +       const char **parent_names;
> +       const struct pmc_clk_data *pmc_data;

Reversed order, please. Usually: assignments at very beginning, longer
first, short later, error code variable last, flags for spin lock --
depends.

> +
> +       pmc_data = dev_get_platdata(&pdev->dev);

> +       if (!pmc_data || !pmc_data->clks)
> +               return -EINVAL;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       parent_names = plt_clk_register_parents(pdev, data, pmc_data->clks);
> +       if (IS_ERR(parent_names))
> +               return PTR_ERR(parent_names);
> +
> +       for (i = 0; i < PMC_CLK_NUM; i++) {
> +               data->clks[i] = plt_clk_register(pdev, i, pmc_data->base,
> +                                                parent_names, data->nparents);
> +               if (IS_ERR(data->clks[i])) {
> +                       err = PTR_ERR(data->clks[i]);
> +                       goto err_unreg_clk_plt;
> +               }
> +       }
> +

> +       for (i = 0; i < data->nparents; i++)
> +               kfree_const(parent_names[i]);
> +       kfree(parent_names);

(1)

> +
> +       dev_set_drvdata(&pdev->dev, data);
> +       return 0;
> +
> +err_unreg_clk_plt:

> +       for (i--; i >= 0; i--)
> +               plt_clk_unregister(data->clks[i]);
> +       plt_clk_unregister_parents(data);

(3)


> +       for (i = 0; i < data->nparents; i++)
> +               kfree_const(parent_names[i]);
> +       kfree(parent_names);

(2)

(1) and (2) -> helper function?

> +       return err;
> +}
> +
> +static int plt_clk_remove(struct platform_device *pdev)
> +{
> +       struct clk_plt_data *data;
> +       int i;
> +
> +       data = dev_get_drvdata(&pdev->dev);
> +       if (!data)
> +               return 0;
> +
> +       for (i = 0; i < PMC_CLK_NUM; i++)
> +               plt_clk_unregister(data->clks[i]);
> +       plt_clk_unregister_parents(data);

(4)

(3) and (4) -> helper function ?

> +       return 0;
> +}
> +
> +static struct platform_driver plt_clk_driver = {
> +       .driver = {
> +               .name = PLT_CLK_DRIVER_NAME,

Better to put such inplace here. You know why? Instead of one git grep
one has to run two in order to find actual driver name.

> +       },
> +       .probe = plt_clk_probe,
> +       .remove = plt_clk_remove,
> +};
> +module_platform_driver(plt_clk_driver);
> +
> +MODULE_DESCRIPTION("Intel Atom platform clocks driver");

> +MODULE_AUTHOR("Irina Tirdea <[e-mail]>");

Be consistent with SoB lines

> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/x86/clk-byt-plt.h b/include/linux/platform_data/x86/clk-byt-plt.h
> new file mode 100644
> index 0000000..e6bca9c
> --- /dev/null
> +++ b/include/linux/platform_data/x86/clk-byt-plt.h
> @@ -0,0 +1,31 @@
> +/*
> + * Intel Atom platform clocks for BayTrail and CherryTrail SoC.
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Irina Tirdea <[e-mail]>

Ditto.
Of course in all cases exceptions are possible (if another author has
done partial stuff)

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#ifndef __CLK_BYT_PLT_H
> +#define __CLK_BYT_PLT_H
> +
> +struct pmc_clk {
> +       const char *name;
> +       unsigned long freq;
> +       const char *parent_name;
> +};
> +
> +struct pmc_clk_data {
> +       void __iomem *base;
> +       const struct pmc_clk *clks;
> +};

Those are definitely do not look like a *platform data* at all.

> +
> +#endif /* __CLK_BYT_PLT_H */

Pierre-Louis Bossart Dec. 13, 2016, 12:15 a.m. UTC | #2

> Thanks for an update I will comment all the patches.
> Here we start.

Thanks Andy for the review. Two quick comments before going further in 
the details later.

>
>> The BayTrail and CherryTrail platforms provide platform clocks
>> through their Power Management Controller (PMC).
>>
>> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
>> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
>> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
>> are available for general system use, where appropriate, and each
>> have Control & Frequency register fields associated with them.
>>
>> Signed-off-by: Irina Tirdea <[e-mail]>
>> Signed-off-by: Pierre-Louis Bossart <[e-mail]>
>
> Who is the actual author? SoB I guess should be either the author, or
> 1st, 2nd, ..., last one who is submitter.

I ported the initial code from Android legacy stuff and Irina ported the 
functionality to the clk framework. It seems appropriate to have both 
signed-offs?

[snip]
>
>> +#include <linux/platform_data/x86/clk-byt-plt.h>

This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
see 
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html

Darren, did we get your proposal right?

>
> Is it indeed platform data? I would not create platform_data/x86
> without strong argument.
> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
> which is old enough and was basically first try of clk stuff on x86)

Andy Shevchenko Dec. 13, 2016, 12:26 a.m. UTC | #3

On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
<[e-mail]> wrote:
>
>> Thanks for an update I will comment all the patches.
>> Here we start.
>
>
> Thanks Andy for the review. Two quick comments before going further in the
> details later.
>
>>
>>> The BayTrail and CherryTrail platforms provide platform clocks
>>> through their Power Management Controller (PMC).
>>>
>>> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
>>> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
>>> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
>>> are available for general system use, where appropriate, and each
>>> have Control & Frequency register fields associated with them.
>>>
>>> Signed-off-by: Irina Tirdea <[e-mail]>
>>> Signed-off-by: Pierre-Louis Bossart
>>> <[e-mail]>
>>
>>
>> Who is the actual author? SoB I guess should be either the author, or
>> 1st, 2nd, ..., last one who is submitter.
>
>
> I ported the initial code from Android legacy stuff and Irina ported the
> functionality to the clk framework. It seems appropriate to have both
> signed-offs?

Yes, but as I mentioned:
1) submitter goes last;
2) SoB lines and Author(s) should reflect actual state of the sources.
If patch has 2 SoBs I'm expecting see different names of Authors in
the source code. *Or* in some cases it's possible to explain in the
commit message why you have former SoB and for what the credit that
person(s) get.

>>> +#include <linux/platform_data/x86/clk-byt-plt.h>
>
>
> This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
> see
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html

Hmm... Thanks for pointing to this I didn't aware about such details.

But... I still insist that is not a platform data at all in both cases.

For clock I would suggest include/linux/clk/ with x86_ prefix.
For the rest I have no strong opinion except trying to avoid
platform_data wording in the path as much as possible.

As an example I could recall DMA engine subsystem where we have

include/linux/platform_data/dma-*.h

and

include/linux/dma/*.h

So, this sounds more to me as

include/linux/x86/pmc_atom.h

> Darren, did we get your proposal right?

>>
>> Is it indeed platform data? I would not create platform_data/x86
>> without strong argument.
>> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
>> which is old enough and was basically first try of clk stuff on x86)

Andy Shevchenko Dec. 13, 2016, 1:16 a.m. UTC | #4

On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
<[e-mail]> wrote:
>
>>> +#include <linux/platform_data/x86/clk-byt-plt.h>
>
>
> This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
> see
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html
>
> Darren, did we get your proposal right?
>
>> Is it indeed platform data? I would not create platform_data/x86
>> without strong argument.
>> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
>> which is old enough and was basically first try of clk stuff on x86)

Looking more into the patch I got another question. Do we really need
a platform driver for that?

That's what I think motivated me for the header location. And that's
why I asked about use of the driver/clock provider in the latter
patch.

If the answer is yes, then I doubt which location is preferable,
otherwise include/clk/x86_*.h looks appropriate.

Sorry if I wasn't clear in the first place.

Stephen Boyd Dec. 13, 2016, 11:25 p.m. UTC | #5

On 12/13, Andy Shevchenko wrote:
> On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <[e-mail]> wrote:
> 
> > --- a/drivers/clk/x86/Makefile
> > +++ b/drivers/clk/x86/Makefile
> > @@ -1,2 +1,5 @@
> >  clk-x86-lpss-objs              := clk-lpt.o
> >  obj-$(CONFIG_X86_INTEL_LPSS)   += clk-x86-lpss.o
> 
> > +ifeq ($(CONFIG_COMMON_CLK), y)
> 
> Hmm... I think (I didn't check) we don't go here otherwise.

We should move this statement to drivers/clk/Makefile around the
x86 line.

> > +                                       void __iomem *base,
> > +                                       const char **parent_names,
> > +                                       int num_parents)
> > +{
> > +       struct clk_plt *pclk;
> > +       struct clk_init_data init;
> > +       int ret;
> > +
> > +       pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
> > +       if (!pclk)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       init.name =  kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id);
> 
> devm_kasprintf()

Please no.

> 
> > +       init.ops = &plt_clk_ops;
> > +       init.flags = 0;
> > +       init.parent_names = parent_names;
> > +       init.num_parents = num_parents;
> > +
> > +       pclk->hw.init = &init;
> > +       pclk->reg = base + id * PMC_CLK_CTL_SIZE;
> > +       spin_lock_init(&pclk->lock);
> > +
> > +       ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
> > +       if (ret)
> > +               goto err_free_init;
> > +
> > +       pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL);
> > +       if (!pclk->lookup) {
> > +               ret = -ENOMEM;
> > +               goto err_free_init;
> > +       }
> > +
> 
> > +       kfree(init.name);
> 
> devm_kfree();

It's all local to this function, devm isn't helping anything.
Having one kfree() would be good though. And using init.name for
the clkdev lookup is probably wrong and should be replaced with
something more generic along with an associated device name.

Pierre-Louis Bossart Dec. 16, 2016, 5:15 a.m. UTC | #6

Hi Stephen,

can you elaborate on the last comment?

thanks,

-Pierre


On 12/13/2016 05:25 PM, Stephen Boyd wrote:
>
>>> +                                       void __iomem *base,
>>> +                                       const char **parent_names,
>>> +                                       int num_parents)
>>> +{
>>> +       struct clk_plt *pclk;
>>> +       struct clk_init_data init;
>>> +       int ret;
>>> +
>>> +       pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
>>> +       if (!pclk)
>>> +               return ERR_PTR(-ENOMEM);
>>> +
>>> +       init.name =  kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id);
>> devm_kasprintf()
> Please no.
>
>>> +       init.ops = &plt_clk_ops;
>>> +       init.flags = 0;
>>> +       init.parent_names = parent_names;
>>> +       init.num_parents = num_parents;
>>> +
>>> +       pclk->hw.init = &init;
>>> +       pclk->reg = base + id * PMC_CLK_CTL_SIZE;
>>> +       spin_lock_init(&pclk->lock);
>>> +
>>> +       ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
>>> +       if (ret)
>>> +               goto err_free_init;
>>> +
>>> +       pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL);
>>> +       if (!pclk->lookup) {
>>> +               ret = -ENOMEM;
>>> +               goto err_free_init;
>>> +       }
>>> +
>>> +       kfree(init.name);
>> devm_kfree();
> It's all local to this function, devm isn't helping anything.
> Having one kfree() would be good though. And using init.name for
> the clkdev lookup is probably wrong and should be replaced with
> something more generic along with an associated device name.
I am not sure I understand this last comment.
init.name is not a constant, it's made of the "pmc_plt_clk_" string 
concatenated with an id which directly maps to which hardware clock is 
registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.
>

Andy Shevchenko Dec. 16, 2016, 8:46 a.m. UTC | #7

On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
<[e-mail]> wrote:
> Hi Stephen,
>
> can you elaborate on the last comment?

Please don't do top posting.

>>> devm_kasprintf()
>>
>> Please no.

That's why I used modal verb "might" instead of "would".

>> It's all local to this function, devm isn't helping anything.
>> Having one kfree() would be good though. And using init.name for
>> the clkdev lookup is probably wrong and should be replaced with
>> something more generic along with an associated device name.
>
> I am not sure I understand this last comment.
> init.name is not a constant, it's made of the "pmc_plt_clk_" string
> concatenated with an id which directly maps to which hardware clock is
> registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.

Giving more thoughts about design and use of this I would propose to
do the following.

1. Create under clock framework something like clk-pmc-atom clock
driver (see, for example, clk-fractional-divider, though this one
should indeed go under x86 folder).
2. In real provider, i.e. pmc_atom, create the necessary clock tree
with *names*.

Scheme with ID is fragile, imagine another version of PMC where
ordering would be mixed up? It's not hypothetical since we used to
have this already in pmc_atom for some registers and bits.

Pierre-Louis Bossart Dec. 16, 2016, 2:57 p.m. UTC | #8

On 12/16/16 2:46 AM, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
> <[e-mail]> wrote:
>> Hi Stephen,
>>
>> can you elaborate on the last comment?
>
> Please don't do top posting.
>
>>>> devm_kasprintf()
>>>
>>> Please no.
>
> That's why I used modal verb "might" instead of "would".
>
>>> It's all local to this function, devm isn't helping anything.
>>> Having one kfree() would be good though. And using init.name for
>>> the clkdev lookup is probably wrong and should be replaced with
>>> something more generic along with an associated device name.
>>
>> I am not sure I understand this last comment.
>> init.name is not a constant, it's made of the "pmc_plt_clk_" string
>> concatenated with an id which directly maps to which hardware clock is
>> registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.
>
> Giving more thoughts about design and use of this I would propose to
> do the following.
>
> 1. Create under clock framework something like clk-pmc-atom clock
> driver (see, for example, clk-fractional-divider, though this one
> should indeed go under x86 folder).

apart from the name the current code already does this with code in 
drivers/clk/x86

> 2. In real provider, i.e. pmc_atom, create the necessary clock tree
> with *names*.
>
> Scheme with ID is fragile, imagine another version of PMC where
> ordering would be mixed up? It's not hypothetical since we used to
> have this already in pmc_atom for some registers and bits.

I don't want to deal with hypothetical stuff happening to legacy 
hardware. If there is a problem at some point, it's no big deal to add a 
platform-dependent lookup table and change the registers being accessed.

Darren Hart Dec. 16, 2016, 6:36 p.m. UTC | #9

On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
> <[e-mail]> wrote:
> >
> >> Thanks for an update I will comment all the patches.
> >> Here we start.
> >
> >
> > Thanks Andy for the review. Two quick comments before going further in the
> > details later.
> >
> >>
> >>> The BayTrail and CherryTrail platforms provide platform clocks
> >>> through their Power Management Controller (PMC).
> >>>
> >>> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
> >>> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
> >>> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
> >>> are available for general system use, where appropriate, and each
> >>> have Control & Frequency register fields associated with them.
> >>>
> >>> Signed-off-by: Irina Tirdea <[e-mail]>
> >>> Signed-off-by: Pierre-Louis Bossart
> >>> <[e-mail]>
> >>
> >>
> >> Who is the actual author? SoB I guess should be either the author, or
> >> 1st, 2nd, ..., last one who is submitter.
> >
> >
> > I ported the initial code from Android legacy stuff and Irina ported the
> > functionality to the clk framework. It seems appropriate to have both
> > signed-offs?
> 
> Yes, but as I mentioned:
> 1) submitter goes last;
> 2) SoB lines and Author(s) should reflect actual state of the sources.
> If patch has 2 SoBs I'm expecting see different names of Authors in
> the source code. *Or* in some cases it's possible to explain in the
> commit message why you have former SoB and for what the credit that
> person(s) get.
> 
> >>> +#include <linux/platform_data/x86/clk-byt-plt.h>
> >
> >
> > This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
> > see
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html
> 
> Hmm... Thanks for pointing to this I didn't aware about such details.
> 
> But... I still insist that is not a platform data at all in both cases.
> 
> For clock I would suggest include/linux/clk/ with x86_ prefix.
> For the rest I have no strong opinion except trying to avoid
> platform_data wording in the path as much as possible.
> 
> As an example I could recall DMA engine subsystem where we have
> 
> include/linux/platform_data/dma-*.h
> 
> and
> 
> include/linux/dma/*.h
> 
> So, this sounds more to me as
> 
> include/linux/x86/pmc_atom.h

There should really be some Documentation about how to choose an include
directory :-)

My understanding is include/linux should be more generic, rather than platform
specific headers. So while platform_data may refer specifically to the platform
bus drivers, it's the closest thing we have to include/platform, which would be
ideal. I would prefer to stick with include/platform_data because:

1) Semantically, it's the closest thing there is
2) include/linux should be for more generic headers related to the OS or
   subsystems
3) It doesn't make sense to create a separate include/platform directory for a
   single header.
4) We don't want to rename platform_data to platform now and change all the
   drivers, but it could be changed later.

Thomas, do you disagree with any of the above?

> 
> > Darren, did we get your proposal right?
> 

Yes, your submission matches the intent from Thomas and I as I understand it.

> >>
> >> Is it indeed platform data? I would not create platform_data/x86
> >> without strong argument.
> >> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
> >> which is old enough and was basically first try of clk stuff on x86)
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>

Andy Shevchenko Dec. 16, 2016, 6:49 p.m. UTC | #10

On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <[e-mail]> wrote:
> On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
>> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
>> <[e-mail]> wrote:

>> For clock I would suggest include/linux/clk/ with x86_ prefix.
>> For the rest I have no strong opinion except trying to avoid
>> platform_data wording in the path as much as possible.
>>
>> As an example I could recall DMA engine subsystem where we have
>>
>> include/linux/platform_data/dma-*.h
>>
>> and
>>
>> include/linux/dma/*.h
>>
>> So, this sounds more to me as
>>
>> include/linux/x86/pmc_atom.h
>
> There should really be some Documentation about how to choose an include
> directory :-)

So true!

> My understanding is include/linux should be more generic, rather than platform
> specific headers. So while platform_data may refer specifically to the platform
> bus drivers, it's the closest thing we have to include/platform, which would be
> ideal. I would prefer to stick with include/platform_data because:
>
> 1) Semantically, it's the closest thing there is
> 2) include/linux should be for more generic headers related to the OS or
>    subsystems
> 3) It doesn't make sense to create a separate include/platform directory for a
>    single header.
> 4) We don't want to rename platform_data to platform now and change all the
>    drivers, but it could be changed later.

My understanding that part like P-Unit, PMIC, PMC, SCU, whatever we
have inside SoC is platform from hardware prospective, but from
software (driver) it doesn't use platform data since it's quite SoC
specific (like CPU model to differentiate). That's why something in
the middle between arch/x86/include/asm and
include/linux/platform_data.

I assume I would be not good in naming schemes, though platform_data
for file which doesn't contain platform data for platform device
sounds a bit confusing to me. Like someone already noticed
include/platform_data is already messy. This might just add another
level of it.

So, what is exactly confuses me is mixing data for *platform devices*
(as represented via *platform driver* -- struct platform_driver)  and
for SoC devices (no struct platform_driver per se).
Maybe I misunderstood something...

Darren Hart Dec. 16, 2016, 7:19 p.m. UTC | #11

On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <[e-mail]> wrote:
> > On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
> >> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
> >> <[e-mail]> wrote:
> 
> >> For clock I would suggest include/linux/clk/ with x86_ prefix.
> >> For the rest I have no strong opinion except trying to avoid
> >> platform_data wording in the path as much as possible.
> >>
> >> As an example I could recall DMA engine subsystem where we have
> >>
> >> include/linux/platform_data/dma-*.h
> >>
> >> and
> >>
> >> include/linux/dma/*.h
> >>
> >> So, this sounds more to me as
> >>
> >> include/linux/x86/pmc_atom.h
> >
> > There should really be some Documentation about how to choose an include
> > directory :-)
> 
> So true!
> 
> > My understanding is include/linux should be more generic, rather than platform
> > specific headers. So while platform_data may refer specifically to the platform
> > bus drivers, it's the closest thing we have to include/platform, which would be
> > ideal. I would prefer to stick with include/platform_data because:
> >
> > 1) Semantically, it's the closest thing there is
> > 2) include/linux should be for more generic headers related to the OS or
> >    subsystems

Scratch #2 from the arguments since it's include/linux/platform_data that we're
talking about here.

> > 3) It doesn't make sense to create a separate include/platform directory for a
> >    single header.
> > 4) We don't want to rename platform_data to platform now and change all the
> >    drivers, but it could be changed later.
> 
> My understanding that part like P-Unit, PMIC, PMC, SCU, whatever we
> have inside SoC is platform from hardware prospective, but from
> software (driver) it doesn't use platform data since it's quite SoC
> specific (like CPU model to differentiate). That's why something in
> the middle between arch/x86/include/asm and
> include/linux/platform_data.
> 
> I assume I would be not good in naming schemes, though platform_data
> for file which doesn't contain platform data for platform device
> sounds a bit confusing to me. Like someone already noticed
> include/platform_data is already messy. This might just add another
> level of it.
> 
> So, what is exactly confuses me is mixing data for *platform devices*
> (as represented via *platform driver* -- struct platform_driver)  and
> for SoC devices (no struct platform_driver per se).
> Maybe I misunderstood something...

You're understanding is correct. We're just applying different values to the
respective merits of each argument.

The options are:

a) include/linux/x86
b) include/linux/platform_data/x86

In my opinion, a) looks like architecture and would be difficult to distinguish
from arch/x86/include. b) on the other hand clearly notes that it is for
platform specific information. If it was platform instead of platform_data, that
would be even better, but that could be a later change. But I think the
confusion over x86 arch in a) is worse than the more subtle (in my opinion)
distinction between "platform" and "platform_data".

I would want x86 maintainer approval before adding a), while b) I'm happy to add
ourselves - and we already have agreement from tglx on that.

To move forward, let's go with b). The new x86 directory clearly separates out
content which will make it trivial to move later if the need arises.

Andy Shevchenko Dec. 16, 2016, 10:29 p.m. UTC | #12

On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart <[e-mail]> wrote:
> On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
>> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <[e-mail]> wrote:
>> > On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
>> >> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart

>> > There should really be some Documentation about how to choose an include
>> > directory :-)
>>
>> So true!

(1)

> The options are:
>
> a) include/linux/x86
> b) include/linux/platform_data/x86

Correct.

> In my opinion, a) looks like architecture and would be difficult to distinguish
> from arch/x86/include. b) on the other hand clearly notes that it is for
> platform specific information. If it was platform instead of platform_data, that
> would be even better, but that could be a later change. But I think the
> confusion over x86 arch in a) is worse than the more subtle (in my opinion)
> distinction between "platform" and "platform_data".
>
> I would want x86 maintainer approval before adding a), while b) I'm happy to add
> ourselves - and we already have agreement from tglx on that.
>
> To move forward, let's go with b).

Let me say I'm not fully satisfied, though for sake of moving forward
I agree with these arguments.

> The new x86 directory clearly separates out
> content which will make it trivial to move later if the need arises.

See (1). I would really appreciate if some agreement and documentation
will be developed.
In that case one of us would really have one serious argument to one
of the sides.

Darren Hart Dec. 16, 2016, 10:58 p.m. UTC | #13

On Sat, Dec 17, 2016 at 12:29:41AM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart <[e-mail]> wrote:
> > On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
> >> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <[e-mail]> wrote:
> >> > On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
> >> >> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
> 
> >> > There should really be some Documentation about how to choose an include
> >> > directory :-)
> >>
> >> So true!
> 
> (1)
> 
> > The options are:
> >
> > a) include/linux/x86
> > b) include/linux/platform_data/x86
> 
> Correct.
> 
> > In my opinion, a) looks like architecture and would be difficult to distinguish
> > from arch/x86/include. b) on the other hand clearly notes that it is for
> > platform specific information. If it was platform instead of platform_data, that
> > would be even better, but that could be a later change. But I think the
> > confusion over x86 arch in a) is worse than the more subtle (in my opinion)
> > distinction between "platform" and "platform_data".
> >
> > I would want x86 maintainer approval before adding a), while b) I'm happy to add
> > ourselves - and we already have agreement from tglx on that.
> >
> > To move forward, let's go with b).
> 
> Let me say I'm not fully satisfied, though for sake of moving forward
> I agree with these arguments.
> 
> > The new x86 directory clearly separates out
> > content which will make it trivial to move later if the need arises.
> 
> See (1). I would really appreciate if some agreement and documentation
> will be developed.
> In that case one of us would really have one serious argument to one
> of the sides.

Agreed. I always prefer to make decisions based on Documented precedent whenever
possible.

Stephen Boyd Dec. 17, 2016, 1:33 a.m. UTC | #14

On 12/15, Pierre-Louis Bossart wrote:
> I am not sure I understand this last comment.
> init.name is not a constant, it's made of the "pmc_plt_clk_" string
> concatenated with an id which directly maps to which hardware clock
> is registered.

That's all fine. We need globally unique strings for clk names in
the framework so things work.

>Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
> argument.

This is the problem. Clients should be calling clk_get() like:

	clk_get(dev, "signal name in datasheet")

where the first argument is the device and the second argument is
some string that is meaningful to the device, not the system as a
whole. The way clkdev is intended is so that the dev argument's
dev_name() is combined with the con_id that matches some signale
name in the datasheet. This way when the same IP is put into some
other chip, the globally unique name doesn't need to change, just
the device name that's registered with the lookup. Obviously this
breaks down quite badly when dev_name() isn't stable. Is that
happening here?

Andy Shevchenko Dec. 17, 2016, 1:57 p.m. UTC | #15

On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[e-mail]> wrote:
> On 12/15, Pierre-Louis Bossart wrote:

>>Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
>> argument.
>
> This is the problem. Clients should be calling clk_get() like:
>
>         clk_get(dev, "signal name in datasheet")
>
> where the first argument is the device and the second argument is
> some string that is meaningful to the device, not the system as a
> whole. The way clkdev is intended is so that the dev argument's
> dev_name() is combined with the con_id that matches some signale
> name in the datasheet. This way when the same IP is put into some
> other chip, the globally unique name doesn't need to change, just
> the device name that's registered with the lookup. Obviously this
> breaks down quite badly when dev_name() isn't stable. Is that
> happening here?

PMC Atom is a PCI device and thus each platform would have different
dev_name(). Do you want to list all in each consumer if consumer wants
to work on all of them or I missed something?

So, the question is how clock getting will look like to work on
currently both CherryTrail and BayTrail.

Mark Brown Dec. 19, 2016, 11:04 a.m. UTC | #16

On Fri, Dec 16, 2016 at 10:36:07AM -0800, Darren Hart wrote:

> My understanding is include/linux should be more generic, rather than platform
> specific headers. So while platform_data may refer specifically to the platform
> bus drivers, it's the closest thing we have to include/platform, which would be
> ideal. I would prefer to stick with include/platform_data because:

It's not specific to the platform bus, it's for use with the platform_data
pointer embedded in struct device that all buses can have - it's
extensively used for things like I2C and SPI for example.  But really it
doesn't matter *that* much.

Pierre-Louis Bossart Dec. 19, 2016, 4:11 p.m. UTC | #17

On 12/17/16 7:57 AM, Andy Shevchenko wrote:
> On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[e-mail]> wrote:
>> On 12/15, Pierre-Louis Bossart wrote:
>
>>> Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
>>> argument.
>>
>> This is the problem. Clients should be calling clk_get() like:
>>
>>         clk_get(dev, "signal name in datasheet")
>>
>> where the first argument is the device and the second argument is
>> some string that is meaningful to the device, not the system as a
>> whole. The way clkdev is intended is so that the dev argument's
>> dev_name() is combined with the con_id that matches some signale
>> name in the datasheet. This way when the same IP is put into some
>> other chip, the globally unique name doesn't need to change, just
>> the device name that's registered with the lookup. Obviously this
>> breaks down quite badly when dev_name() isn't stable. Is that
>> happening here?
>
> PMC Atom is a PCI device and thus each platform would have different
> dev_name(). Do you want to list all in each consumer if consumer wants
> to work on all of them or I missed something?
>
> So, the question is how clock getting will look like to work on
> currently both CherryTrail and BayTrail.

The name pmc_plt_clk_<n> follows the data sheet specification, where 
this convention is suggested:
   PLT_CLK[2:0] - Camera
   PLT_CLK[3] - Audio Codec
   PLT_CLK[4] -
   PLT_CLK[5] - COMMs

These clocks are not internal but are made available to external 
components through dedicated physical pins on the package, this external 
visibility limits the scope for confusions, variations. I have not seen 
any skews where these clocks and pins were changed at all.

Stephen Boyd Dec. 21, 2016, 11:05 p.m. UTC | #18

On 12/19, Pierre-Louis Bossart wrote:
> On 12/17/16 7:57 AM, Andy Shevchenko wrote:
> >On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[e-mail]> wrote:
> >>On 12/15, Pierre-Louis Bossart wrote:
> >
> >>>Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
> >>>argument.
> >>
> >>This is the problem. Clients should be calling clk_get() like:
> >>
> >>        clk_get(dev, "signal name in datasheet")
> >>
> >>where the first argument is the device and the second argument is
> >>some string that is meaningful to the device, not the system as a
> >>whole. The way clkdev is intended is so that the dev argument's
> >>dev_name() is combined with the con_id that matches some signale
> >>name in the datasheet. This way when the same IP is put into some
> >>other chip, the globally unique name doesn't need to change, just
> >>the device name that's registered with the lookup. Obviously this
> >>breaks down quite badly when dev_name() isn't stable. Is that
> >>happening here?
> >
> >PMC Atom is a PCI device and thus each platform would have different
> >dev_name(). Do you want to list all in each consumer if consumer wants
> >to work on all of them or I missed something?
> >
> >So, the question is how clock getting will look like to work on
> >currently both CherryTrail and BayTrail.
> 
> The name pmc_plt_clk_<n> follows the data sheet specification, where
> this convention is suggested:
>   PLT_CLK[2:0] - Camera
>   PLT_CLK[3] - Audio Codec
>   PLT_CLK[4] -
>   PLT_CLK[5] - COMMs
> 
> These clocks are not internal but are made available to external
> components through dedicated physical pins on the package, this
> external visibility limits the scope for confusions, variations. I
> have not seen any skews where these clocks and pins were changed at
> all.

Ok, by clkdev design if a device is passed but there isn't a
match in the lookup table it allows it to match based solely on
the connection id. Given that the connection id is globally
unique this will work.

Hopefully we don't have two of these devices with pmc_plt_clk_<n>
signals in a single system though. Then having the device name
would help differentiate between the two. And then it may make
sense to have some sort of ACPI lookup system, similar to how we
have lookups for clks in DT.

Pierre-Louis Bossart Dec. 22, 2016, 1:07 a.m. UTC | #19

On 12/21/16 5:05 PM, Stephen Boyd wrote:
> On 12/19, Pierre-Louis Bossart wrote:
>> On 12/17/16 7:57 AM, Andy Shevchenko wrote:
>>> On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[e-mail]> wrote:
>>>> On 12/15, Pierre-Louis Bossart wrote:
>>>
>>>>> Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
>>>>> argument.
>>>>
>>>> This is the problem. Clients should be calling clk_get() like:
>>>>
>>>>        clk_get(dev, "signal name in datasheet")
>>>>
>>>> where the first argument is the device and the second argument is
>>>> some string that is meaningful to the device, not the system as a
>>>> whole. The way clkdev is intended is so that the dev argument's
>>>> dev_name() is combined with the con_id that matches some signale
>>>> name in the datasheet. This way when the same IP is put into some
>>>> other chip, the globally unique name doesn't need to change, just
>>>> the device name that's registered with the lookup. Obviously this
>>>> breaks down quite badly when dev_name() isn't stable. Is that
>>>> happening here?
>>>
>>> PMC Atom is a PCI device and thus each platform would have different
>>> dev_name(). Do you want to list all in each consumer if consumer wants
>>> to work on all of them or I missed something?
>>>
>>> So, the question is how clock getting will look like to work on
>>> currently both CherryTrail and BayTrail.
>>
>> The name pmc_plt_clk_<n> follows the data sheet specification, where
>> this convention is suggested:
>>   PLT_CLK[2:0] - Camera
>>   PLT_CLK[3] - Audio Codec
>>   PLT_CLK[4] -
>>   PLT_CLK[5] - COMMs
>>
>> These clocks are not internal but are made available to external
>> components through dedicated physical pins on the package, this
>> external visibility limits the scope for confusions, variations. I
>> have not seen any skews where these clocks and pins were changed at
>> all.
>
> Ok, by clkdev design if a device is passed but there isn't a
> match in the lookup table it allows it to match based solely on
> the connection id. Given that the connection id is globally
> unique this will work.
>
> Hopefully we don't have two of these devices with pmc_plt_clk_<n>
> signals in a single system though. Then having the device name
> would help differentiate between the two. And then it may make
> sense to have some sort of ACPI lookup system, similar to how we
> have lookups for clks in DT.

So in short we keep the existing solution for now and will only use the 
device name if and when the pmc_plt_clk_<n> identifier is no longer 
unique due to hardware changes. Did I get this right?

Stephen Boyd Dec. 22, 2016, 6:29 p.m. UTC | #20

On 12/21/2016 05:07 PM, Pierre-Louis Bossart wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:
>>
>> Ok, by clkdev design if a device is passed but there isn't a
>> match in the lookup table it allows it to match based solely on
>> the connection id. Given that the connection id is globally
>> unique this will work.
>>
>> Hopefully we don't have two of these devices with pmc_plt_clk_<n>
>> signals in a single system though. Then having the device name
>> would help differentiate between the two. And then it may make
>> sense to have some sort of ACPI lookup system, similar to how we
>> have lookups for clks in DT.
>
> So in short we keep the existing solution for now and will only use
> the device name if and when the pmc_plt_clk_<n> identifier is no
> longer unique due to hardware changes. Did I get this right?

Ok.

Andy Shevchenko Dec. 22, 2016, 6:42 p.m. UTC | #21

On Thu, Dec 22, 2016 at 3:07 AM, Pierre-Louis Bossart
<[e-mail]> wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:

>>> The name pmc_plt_clk_<n> follows the data sheet specification, where
>>> this convention is suggested:
>>>   PLT_CLK[2:0] - Camera
>>>   PLT_CLK[3] - Audio Codec
>>>   PLT_CLK[4] -
>>>   PLT_CLK[5] - COMMs

By the way, would I suggest to use same prefix as provider, i.e.
pmc_atom_plt_clk_%d ?

Pierre-Louis Bossart Jan. 5, 2017, 12:54 a.m. UTC | #22

>>>> this convention is suggested:
>>>>    PLT_CLK[2:0] - Camera
>>>>    PLT_CLK[3] - Audio Codec
>>>>    PLT_CLK[4] -
>>>>    PLT_CLK[5] - COMMs
> By the way, would I suggest to use same prefix as provider, i.e.
> pmc_atom_plt_clk_%d ?
I tried this suggestion and it doesn't work unfortunately. It looks like 
the struct clk_lookup_alloc is limited to 16 chars for the connector_id.
"pmc_a_plt_clk_" works but that's not really helping.
The suggestion would also require a patch on the audio side since the 
use of pmc_plt_clk_3 was already merged.
-> no change.

Patch

diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile
index 0478138..bf7c132 100644
--- a/drivers/clk/x86/Makefile
+++ b/drivers/clk/x86/Makefile
@@ -1,2 +1,5 @@ 
 clk-x86-lpss-objs		:= clk-lpt.o
 obj-$(CONFIG_X86_INTEL_LPSS)	+= clk-x86-lpss.o
+ifeq ($(CONFIG_COMMON_CLK), y)
+obj-$(CONFIG_PMC_ATOM)		+= clk-byt-plt.o
+endif
diff --git a/drivers/clk/x86/clk-byt-plt.c b/drivers/clk/x86/clk-byt-plt.c
new file mode 100644
index 0000000..2303e0d
--- /dev/null
+++ b/drivers/clk/x86/clk-byt-plt.c
@@ -0,0 +1,380 @@ 
+/*
+ * Intel Atom platform clocks driver for BayTrail and CherryTrail SoC.
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Irina Tirdea <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/clkdev.h>
+#include <linux/platform_data/x86/clk-byt-plt.h>
+
+#define PLT_CLK_NAME_BASE	"pmc_plt_clk_"
+#define PLT_CLK_DRIVER_NAME	"clk-byt-plt"
+
+#define PMC_CLK_CTL_SIZE	4
+#define PMC_CLK_NUM		6
+#define PMC_MASK_CLK_CTL	GENMASK(1, 0)
+#define PMC_MASK_CLK_FREQ	BIT(2)
+#define PMC_CLK_CTL_GATED_ON_D3	0x0
+#define PMC_CLK_CTL_FORCE_ON	0x1
+#define PMC_CLK_CTL_FORCE_OFF	0x2
+#define PMC_CLK_CTL_RESERVED	0x3
+#define PMC_CLK_FREQ_XTAL	0x0	/* 25 MHz */
+#define PMC_CLK_FREQ_PLL	0x4	/* 19.2 MHz */
+
+struct clk_plt_fixed {
+	struct clk_hw *clk;
+	struct clk_lookup *lookup;
+};
+
+struct clk_plt {
+	struct clk_hw hw;
+	void __iomem *reg;
+	struct clk_lookup *lookup;
+	spinlock_t lock;
+};
+
+#define to_clk_plt(_hw) container_of(_hw, struct clk_plt, hw)
+
+struct clk_plt_data {
+	struct clk_plt_fixed **parents;
+	u8 nparents;
+	struct clk_plt *clks[PMC_CLK_NUM];
+};
+
+static inline int plt_reg_to_parent(int reg)
+{
+	switch (reg & PMC_MASK_CLK_FREQ) {
+	case PMC_CLK_FREQ_XTAL:
+		return 0;	/* index 0 in parents[] */
+	case PMC_CLK_FREQ_PLL:
+		return 1;	/* index 1 in parents[] */
+	}
+
+	return 0;
+}
+
+static inline int plt_parent_to_reg(int index)
+{
+	switch (index) {
+	case 0:	/* index 0 in parents[] */
+		return PMC_CLK_FREQ_XTAL;
+	case 1:	/* index 0 in parents[] */
+		return PMC_CLK_FREQ_PLL;
+	}
+
+	return PMC_CLK_FREQ_XTAL;
+}
+
+static inline int plt_reg_to_enabled(int reg)
+{
+	switch (reg & PMC_MASK_CLK_CTL) {
+	case PMC_CLK_CTL_GATED_ON_D3:
+	case PMC_CLK_CTL_FORCE_ON:
+		return 1;	/* enabled */
+	case PMC_CLK_CTL_FORCE_OFF:
+	case PMC_CLK_CTL_RESERVED:
+	default:
+		return 0;	/* disabled */
+	}
+}
+
+static void plt_clk_reg_update(struct clk_plt *clk, u32 mask, u32 val)
+{
+	u32 orig, tmp;
+	unsigned long flags = 0;
+
+	spin_lock_irqsave(&clk->lock, flags);
+
+	orig = readl(clk->reg);
+
+	tmp = orig & ~mask;
+	tmp |= val & mask;
+
+	if (tmp != orig)
+		writel(tmp, clk->reg);
+
+	spin_unlock_irqrestore(&clk->lock, flags);
+}
+
+static int plt_clk_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+
+	plt_clk_reg_update(clk, PMC_MASK_CLK_FREQ, plt_parent_to_reg(index));
+
+	return 0;
+}
+
+static u8 plt_clk_get_parent(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+	u32 value;
+
+	value = readl(clk->reg);
+
+	return plt_reg_to_parent(value);
+}
+
+static int plt_clk_enable(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+
+	plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_ON);
+
+	return 0;
+}
+
+static void plt_clk_disable(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+
+	plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_OFF);
+}
+
+static int plt_clk_is_enabled(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+	u32 value;
+
+	value = readl(clk->reg);
+
+	return plt_reg_to_enabled(value);
+}
+
+static const struct clk_ops plt_clk_ops = {
+	.enable = plt_clk_enable,
+	.disable = plt_clk_disable,
+	.is_enabled = plt_clk_is_enabled,
+	.get_parent = plt_clk_get_parent,
+	.set_parent = plt_clk_set_parent,
+	.determine_rate = __clk_mux_determine_rate,
+};
+
+static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,
+					void __iomem *base,
+					const char **parent_names,
+					int num_parents)
+{
+	struct clk_plt *pclk;
+	struct clk_init_data init;
+	int ret;
+
+	pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
+	if (!pclk)
+		return ERR_PTR(-ENOMEM);
+
+	init.name =  kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id);
+	init.ops = &plt_clk_ops;
+	init.flags = 0;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	pclk->hw.init = &init;
+	pclk->reg = base + id * PMC_CLK_CTL_SIZE;
+	spin_lock_init(&pclk->lock);
+
+	ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
+	if (ret)
+		goto err_free_init;
+
+	pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL);
+	if (!pclk->lookup) {
+		ret = -ENOMEM;
+		goto err_free_init;
+	}
+
+	kfree(init.name);
+
+	return pclk;
+
+err_free_init:
+	kfree(init.name);
+	return ERR_PTR(ret);
+}
+
+static void plt_clk_unregister(struct clk_plt *pclk)
+{
+	clkdev_drop(pclk->lookup);
+}
+
+static struct clk_plt_fixed *plt_clk_register_fixed_rate(struct platform_device *pdev,
+						 const char *name,
+						 const char *parent_name,
+						 unsigned long fixed_rate)
+{
+	struct clk_plt_fixed *pclk;
+	int ret = 0;
+
+	pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
+	if (!pclk)
+		return ERR_PTR(-ENOMEM);
+
+	pclk->clk = clk_hw_register_fixed_rate(&pdev->dev, name, parent_name,
+					       0, fixed_rate);
+	if (IS_ERR(pclk->clk))
+		return ERR_CAST(pclk->clk);
+
+	pclk->lookup = clkdev_hw_create(pclk->clk, name, NULL);
+	if (!pclk->lookup) {
+		ret = -ENOMEM;
+		goto err_clk_unregister;
+	}
+
+	return pclk;
+
+err_clk_unregister:
+	clk_hw_unregister_fixed_rate(pclk->clk);
+	return ERR_PTR(ret);
+}
+
+static void plt_clk_unregister_fixed_rate(struct clk_plt_fixed *pclk)
+{
+	clkdev_drop(pclk->lookup);
+	clk_hw_unregister_fixed_rate(pclk->clk);
+}
+
+static const char **plt_clk_register_parents(struct platform_device *pdev,
+					     struct clk_plt_data *data,
+					     const struct pmc_clk *clks)
+{
+	const char **parent_names;
+	int i, err;
+
+	data->nparents = 0;
+	while (clks[data->nparents].name)
+		data->nparents++;
+
+	data->parents = devm_kcalloc(&pdev->dev, data->nparents,
+				     sizeof(*data->parents), GFP_KERNEL);
+	if (!data->parents) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	parent_names = kcalloc(data->nparents, sizeof(*parent_names),
+			       GFP_KERNEL);
+	if (!parent_names) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	for (i = 0; i < data->nparents; i++) {
+		data->parents[i] =
+			plt_clk_register_fixed_rate(pdev, clks[i].name,
+						    clks[i].parent_name,
+						    clks[i].freq);
+		if (IS_ERR(data->parents[i])) {
+			err = PTR_ERR(data->parents[i]);
+			goto err_unreg;
+		}
+		parent_names[i] = kstrdup_const(clks[i].name, GFP_KERNEL);
+	}
+
+	return parent_names;
+
+err_unreg:
+	for (i--; i >= 0; i--) {
+		plt_clk_unregister_fixed_rate(data->parents[i]);
+		kfree_const(parent_names[i]);
+	}
+	kfree(parent_names);
+err_out:
+	data->nparents = 0;
+	return ERR_PTR(err);
+}
+
+static void plt_clk_unregister_parents(struct clk_plt_data *data)
+{
+	int i;
+
+	for (i = 0; i < data->nparents; i++)
+		plt_clk_unregister_fixed_rate(data->parents[i]);
+}
+
+static int plt_clk_probe(struct platform_device *pdev)
+{
+	struct clk_plt_data *data;
+	int i, err;
+	const char **parent_names;
+	const struct pmc_clk_data *pmc_data;
+
+	pmc_data = dev_get_platdata(&pdev->dev);
+	if (!pmc_data || !pmc_data->clks)
+		return -EINVAL;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	parent_names = plt_clk_register_parents(pdev, data, pmc_data->clks);
+	if (IS_ERR(parent_names))
+		return PTR_ERR(parent_names);
+
+	for (i = 0; i < PMC_CLK_NUM; i++) {
+		data->clks[i] = plt_clk_register(pdev, i, pmc_data->base,
+						 parent_names, data->nparents);
+		if (IS_ERR(data->clks[i])) {
+			err = PTR_ERR(data->clks[i]);
+			goto err_unreg_clk_plt;
+		}
+	}
+
+	for (i = 0; i < data->nparents; i++)
+		kfree_const(parent_names[i]);
+	kfree(parent_names);
+
+	dev_set_drvdata(&pdev->dev, data);
+	return 0;
+
+err_unreg_clk_plt:
+	for (i--; i >= 0; i--)
+		plt_clk_unregister(data->clks[i]);
+	plt_clk_unregister_parents(data);
+	for (i = 0; i < data->nparents; i++)
+		kfree_const(parent_names[i]);
+	kfree(parent_names);
+	return err;
+}
+
+static int plt_clk_remove(struct platform_device *pdev)
+{
+	struct clk_plt_data *data;
+	int i;
+
+	data = dev_get_drvdata(&pdev->dev);
+	if (!data)
+		return 0;
+
+	for (i = 0; i < PMC_CLK_NUM; i++)
+		plt_clk_unregister(data->clks[i]);
+	plt_clk_unregister_parents(data);
+	return 0;
+}
+
+static struct platform_driver plt_clk_driver = {
+	.driver = {
+		.name = PLT_CLK_DRIVER_NAME,
+	},
+	.probe = plt_clk_probe,
+	.remove = plt_clk_remove,
+};
+module_platform_driver(plt_clk_driver);
+
+MODULE_DESCRIPTION("Intel Atom platform clocks driver");
+MODULE_AUTHOR("Irina Tirdea <[e-mail]>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/x86/clk-byt-plt.h b/include/linux/platform_data/x86/clk-byt-plt.h
new file mode 100644
index 0000000..e6bca9c
--- /dev/null
+++ b/include/linux/platform_data/x86/clk-byt-plt.h
@@ -0,0 +1,31 @@ 
+/*
+ * Intel Atom platform clocks for BayTrail and CherryTrail SoC.
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Irina Tirdea <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#ifndef __CLK_BYT_PLT_H
+#define __CLK_BYT_PLT_H
+
+struct pmc_clk {
+	const char *name;
+	unsigned long freq;
+	const char *parent_name;
+};
+
+struct pmc_clk_data {
+	void __iomem *base;
+	const struct pmc_clk *clks;
+};
+
+#endif /* __CLK_BYT_PLT_H */