On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <irina.tirdea@xxxxxxxxx> 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 <irina.tirdea@xxxxxxxxx>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
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 <irina.tirdea@xxxxxxxxx>
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 <irina.tirdea@xxxxxxxxx>");
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 <irina.tirdea@xxxxxxxxx>
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 */
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
