[alsa-devel] [PATCH v6 0/3] Add platform clock for BayTrail platforms
These patches specifically enable the audio MCLK required by Baytrail CR devices. It is the remaining part of a bigger set of patches (already merged in Mark's tree) that enable sound for Baytrail CR devices (especially Asus T100TAF) [1]. They include the clock driver and enabling the clock in the pmc_atom code (along with moving of the non-architectural pmc_atom driver code into drivers/platform/x86).
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111704.html
Changes from v5: - fix build error reported by kbuild test robot - split the clk driver code from x86 platform changes
Changes from v4: - move the pmc_atom driver from arch/x86/platform/atom to drivers/platform/x86
Changes from v3: - replace devm_kzalloc with devm_kcalloc - add x86 architecture maintainers
Changes from v2: - move clk platform data structures to a separate include file - store clk_hw pointer for the fixed rate clocks
Changes from v1: - register the clk device as child of pmc device - pass iomem pointer from pmc driver to clk driver to avoid using pmc_atom_read()/write() and use readl/writel API instead - use devm_clk_hw_register/clkdev_hw_create instead of clk_register/clkdev_create
Irina Tirdea (3): clk: x86: Add Atom PMC platform clocks arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 platform/x86: Enable Atom PMC platform clocks
arch/x86/Kconfig | 4 - arch/x86/platform/atom/Makefile | 1 - drivers/acpi/acpi_lpss.c | 2 +- drivers/clk/x86/Makefile | 3 + drivers/clk/x86/clk-byt-plt.c | 380 +++++++++++++++++++++ drivers/platform/x86/Kconfig | 5 + drivers/platform/x86/Makefile | 1 + .../atom => drivers/platform/x86}/pmc_atom.c | 81 ++++- include/linux/platform_data/x86/clk-byt-plt.h | 31 ++ .../linux/platform_data/x86}/pmc_atom.h | 3 + 10 files changed, 500 insertions(+), 11 deletions(-) create mode 100644 drivers/clk/x86/clk-byt-plt.c rename {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c (88%) create mode 100644 include/linux/platform_data/x86/clk-byt-plt.h rename {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h (98%)
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@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- 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
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 irina.tirdea@intel.com + * + * 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 irina.tirdea@intel.com"); +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@intel.com + * + * 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 */
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea irina.tirdea@intel.com 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@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
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@intel.com
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@intel.com");
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@intel.com
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 */
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 irina.tirdea@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
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.htm...
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)
On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com 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 irina.tirdea@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
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.htm...
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)
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 pierre-louis.bossart@linux.intel.com 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 irina.tirdea@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
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:
- submitter goes last;
- 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.htm...
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
On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart dvhart@infradead.org 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 pierre-louis.bossart@linux.intel.com 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:
- Semantically, it's the closest thing there is
- include/linux should be for more generic headers related to the OS or subsystems
- It doesn't make sense to create a separate include/platform directory for a single header.
- 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...
On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart dvhart@infradead.org 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 pierre-louis.bossart@linux.intel.com 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:
- Semantically, it's the closest thing there is
- 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.
- It doesn't make sense to create a separate include/platform directory for a single header.
- 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.
On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart dvhart@infradead.org 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 dvhart@infradead.org 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.
On Sat, Dec 17, 2016 at 12:29:41AM +0200, Andy Shevchenko wrote:
On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart dvhart@infradead.org 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 dvhart@infradead.org 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.
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.
On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com 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.htm...
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.
On 12/13, Andy Shevchenko wrote:
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea irina.tirdea@intel.com 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.
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.
On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com 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.
On 12/16/16 2:46 AM, Andy Shevchenko wrote:
On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com 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.
- 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
- 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.
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?
On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd sboyd@codeaurora.org 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.
On 12/17/16 7:57 AM, Andy Shevchenko wrote:
On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd sboyd@codeaurora.org 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.
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 sboyd@codeaurora.org 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.
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 sboyd@codeaurora.org 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?
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.
On Thu, Dec 22, 2016 at 3:07 AM, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com 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 ?
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.
The pmc_atom driver does not contain any architecture specific code. It only enables the SOC Power Management Controller Driver for BayTrail and CherryTrail platforms.
Move the pmc_atom driver from arch/x86/platform/atom to drivers/platform/x86.
Signed-off-by: Irina Tirdea irina.tirdea@intel.com --- arch/x86/Kconfig | 4 ---- arch/x86/platform/atom/Makefile | 1 - drivers/acpi/acpi_lpss.c | 2 +- drivers/platform/x86/Kconfig | 4 ++++ drivers/platform/x86/Makefile | 1 + {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c | 3 +-- {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0 7 files changed, 7 insertions(+), 8 deletions(-) rename {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c (99%) rename {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h (100%)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index bada636..5a009f0 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2753,10 +2753,6 @@ config X86_DMA_REMAP bool depends on STA2X11
-config PMC_ATOM - def_bool y - depends on PCI - source "net/Kconfig"
source "drivers/Kconfig" diff --git a/arch/x86/platform/atom/Makefile b/arch/x86/platform/atom/Makefile index 40983f5..57be88f 100644 --- a/arch/x86/platform/atom/Makefile +++ b/arch/x86/platform/atom/Makefile @@ -1,2 +1 @@ -obj-$(CONFIG_PMC_ATOM) += pmc_atom.o obj-$(CONFIG_PUNIT_ATOM_DEBUG) += punit_atom_debug.o diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c index 373657f..3e4c566 100644 --- a/drivers/acpi/acpi_lpss.c +++ b/drivers/acpi/acpi_lpss.c @@ -18,6 +18,7 @@ #include <linux/mutex.h> #include <linux/platform_device.h> #include <linux/platform_data/clk-lpss.h> +#include <linux/platform_data/x86/pmc_atom.h> #include <linux/pm_domain.h> #include <linux/pm_runtime.h> #include <linux/delay.h> @@ -31,7 +32,6 @@ #include <asm/cpu_device_id.h> #include <asm/intel-family.h> #include <asm/iosf_mbi.h> -#include <asm/pmc_atom.h>
#define LPSS_ADDR(desc) ((unsigned long)&desc)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index b8a21d7..21dce1e 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1028,3 +1028,7 @@ config INTEL_TELEMETRY directly via debugfs files. Various tools may use this interface for SoC state monitoring. endif # X86_PLATFORM_DEVICES + +config PMC_ATOM + def_bool y + depends on PCI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 2efa86d..8568d74 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -71,3 +71,4 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ intel_telemetry_pltdrv.o \ intel_telemetry_debugfs.o obj-$(CONFIG_INTEL_PMC_CORE) += intel_pmc_core.o +obj-$(CONFIG_PMC_ATOM) += pmc_atom.o diff --git a/arch/x86/platform/atom/pmc_atom.c b/drivers/platform/x86/pmc_atom.c similarity index 99% rename from arch/x86/platform/atom/pmc_atom.c rename to drivers/platform/x86/pmc_atom.c index 964ff4f..b53fbc1 100644 --- a/arch/x86/platform/atom/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -21,8 +21,7 @@ #include <linux/debugfs.h> #include <linux/seq_file.h> #include <linux/io.h> - -#include <asm/pmc_atom.h> +#include <linux/platform_data/x86/pmc_atom.h>
struct pmc_bit_map { const char *name; diff --git a/arch/x86/include/asm/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h similarity index 100% rename from arch/x86/include/asm/pmc_atom.h rename to include/linux/platform_data/x86/pmc_atom.h
I have been told I have to send my comments here instead of our internal ML. I didn't fast enough to comment that during v5. So do it right now.
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea irina.tirdea@intel.com wrote:
The pmc_atom driver does not contain any architecture specific code. It only enables the SOC Power Management Controller Driver
SOC -> SoC Driver -> driver
for BayTrail and CherryTrail platforms.
Move the pmc_atom driver from arch/x86/platform/atom to drivers/platform/x86.
Signed-off-by: Irina Tirdea irina.tirdea@intel.com
arch/x86/Kconfig | 4 ---- arch/x86/platform/atom/Makefile | 1 - drivers/acpi/acpi_lpss.c | 2 +- drivers/platform/x86/Kconfig | 4 ++++ drivers/platform/x86/Makefile | 1 + {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c | 3 +--
{arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0
No, it's not a *platform data*.
Other that that looks good to me.
On Tue, Dec 13, 2016 at 01:43:31AM +0200, Andy Shevchenko wrote:
I have been told I have to send my comments here instead of our internal ML. I didn't fast enough to comment that during v5. So do it right now.
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea irina.tirdea@intel.com wrote:
The pmc_atom driver does not contain any architecture specific code. It only enables the SOC Power Management Controller Driver
SOC -> SoC Driver -> driver
for BayTrail and CherryTrail platforms.
Move the pmc_atom driver from arch/x86/platform/atom to drivers/platform/x86.
Signed-off-by: Irina Tirdea irina.tirdea@intel.com
arch/x86/Kconfig | 4 ---- arch/x86/platform/atom/Makefile | 1 - drivers/acpi/acpi_lpss.c | 2 +- drivers/platform/x86/Kconfig | 4 ++++ drivers/platform/x86/Makefile | 1 + {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c | 3 +--
{arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0
No, it's not a *platform data*.
Other that that looks good to me.
Where would you recommend instead? It needs a place to serve both acpi_lpss.c and pmc_atom.c. The include/linux/platform_data/x86 location doesn't seem too strange as it supports a "platform" driver in the sense that pmc_atom is a platform driver.
On Fri, Dec 16, 2016 at 8:20 PM, Darren Hart dvhart@infradead.org wrote:
On Tue, Dec 13, 2016 at 01:43:31AM +0200, Andy Shevchenko wrote:
I have been told I have to send my comments here instead of our internal ML. I didn't fast enough to comment that during v5. So do it right now.
{arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0
No, it's not a *platform data*.
Other that that looks good to me.
Where would you recommend instead? It needs a place to serve both acpi_lpss.c and pmc_atom.c. The include/linux/platform_data/x86 location doesn't seem too strange as it supports a "platform" driver in the sense that pmc_atom is a platform driver.
include/linux/x86 ?
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 an a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks are available for general system use, where appropriate. For example, the usage for platform clocks suggested in the datasheet is the following: PLT_CLK[2:0] - Camera PLT_CLK[3] - Audio Codec PLT_CLK[4] - PLT_CLK[5] - COMMs
Signed-off-by: Irina Tirdea irina.tirdea@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- drivers/platform/x86/Kconfig | 1 + drivers/platform/x86/pmc_atom.c | 78 ++++++++++++++++++++++++++++-- include/linux/platform_data/x86/pmc_atom.h | 3 ++ 3 files changed, 79 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 21dce1e..c1b07ed 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1032,3 +1032,4 @@ endif # X86_PLATFORM_DEVICES config PMC_ATOM def_bool y depends on PCI + select COMMON_CLK diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index b53fbc1..324c44f 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -22,6 +22,8 @@ #include <linux/seq_file.h> #include <linux/io.h> #include <linux/platform_data/x86/pmc_atom.h> +#include <linux/platform_device.h> +#include <linux/platform_data/x86/clk-byt-plt.h>
struct pmc_bit_map { const char *name; @@ -36,6 +38,11 @@ struct pmc_reg_map { const struct pmc_bit_map *pss; };
+struct pmc_data { + const struct pmc_reg_map *map; + const struct pmc_clk *clks; +}; + struct pmc_dev { u32 base_addr; void __iomem *regmap; @@ -49,6 +56,29 @@ struct pmc_dev { static struct pmc_dev pmc_device; static u32 acpi_base_addr;
+static const struct pmc_clk byt_clks[] = { + { + .name = "xtal", + .freq = 25000000, + .parent_name = NULL, + }, + { + .name = "pll", + .freq = 19200000, + .parent_name = "xtal", + }, + {}, +}; + +static const struct pmc_clk cht_clks[] = { + { + .name = "xtal", + .freq = 19200000, + .parent_name = NULL, + }, + {}, +}; + static const struct pmc_bit_map d3_sts_0_map[] = { {"LPSS1_F0_DMA", BIT_LPSS1_F0_DMA}, {"LPSS1_F1_PWM1", BIT_LPSS1_F1_PWM1}, @@ -168,6 +198,16 @@ struct pmc_dev { .pss = cht_pss_map, };
+static const struct pmc_data byt_data = { + .map = &byt_reg_map, + .clks = byt_clks, +}; + +static const struct pmc_data cht_data = { + .map = &cht_reg_map, + .clks = cht_clks, +}; + static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset) { return readl(pmc->regmap + reg_offset); @@ -381,10 +421,36 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) } #endif /* CONFIG_DEBUG_FS */
+static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, + const struct pmc_data *pmc_data) +{ + struct platform_device *clkdev; + struct pmc_clk_data *clk_data; + + clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); + if (!clk_data) + return -ENOMEM; + + clk_data->base = pmc_regmap + PMC_CLK_CTL_0; + clk_data->clks = pmc_data->clks; + + clkdev = platform_device_register_data(&pdev->dev, "clk-byt-plt", -1, + clk_data, sizeof(*clk_data)); + if (IS_ERR(clkdev)) { + kfree(clk_data); + return PTR_ERR(clkdev); + } + + kfree(clk_data); + + return 0; +} + static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) { struct pmc_dev *pmc = &pmc_device; - const struct pmc_reg_map *map = (struct pmc_reg_map *)ent->driver_data; + const struct pmc_data *data = (struct pmc_data *)ent->driver_data; + const struct pmc_reg_map *map = data->map; int ret;
/* Obtain ACPI base address */ @@ -413,6 +479,12 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) dev_warn(&pdev->dev, "debugfs register failed\n");
+ /* Register platform clocks - PMC_PLT_CLK [5:0] */ + ret = pmc_setup_clks(pdev, pmc->regmap, data); + if (ret) + dev_warn(&pdev->dev, "platform clocks register failed: %d\n", + ret); + pmc->init = true; return ret; } @@ -423,8 +495,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) * used by pci_match_id() call below. */ static const struct pci_device_id pmc_pci_ids[] = { - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_VLV_PMC), (kernel_ulong_t)&byt_reg_map }, - { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_CHT_PMC), (kernel_ulong_t)&cht_reg_map }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_VLV_PMC), (kernel_ulong_t)&byt_data }, + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_CHT_PMC), (kernel_ulong_t)&cht_data }, { 0, }, };
diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h index aa8744c..2d310cf 100644 --- a/include/linux/platform_data/x86/pmc_atom.h +++ b/include/linux/platform_data/x86/pmc_atom.h @@ -50,6 +50,9 @@ BIT_ORED_DEDICATED_IRQ_GPSC | \ BIT_SHARED_IRQ_GPSS)
+/* Platform clock control registers */ +#define PMC_CLK_CTL_0 0x60 + /* The timers acumulate time spent in sleep state */ #define PMC_S0IR_TMR 0x80 #define PMC_S0I1_TMR 0x84
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea irina.tirdea@intel.com wrote:
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 an a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks are available for general system use, where appropriate. For example, the usage for platform clocks suggested in the datasheet is the following: PLT_CLK[2:0] - Camera PLT_CLK[3] - Audio Codec PLT_CLK[4] - PLT_CLK[5] - COMMs
Signed-off-by: Irina Tirdea irina.tirdea@intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Same comments as per patch 1/3.
drivers/platform/x86/Kconfig | 1 + drivers/platform/x86/pmc_atom.c | 78 ++++++++++++++++++++++++++++--
include/linux/platform_data/x86/pmc_atom.h | 3 ++
Same.
3 files changed, 79 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 21dce1e..c1b07ed 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1032,3 +1032,4 @@ endif # X86_PLATFORM_DEVICES config PMC_ATOM def_bool y depends on PCI
select COMMON_CLKdiff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index b53fbc1..324c44f 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -22,6 +22,8 @@ #include <linux/seq_file.h> #include <linux/io.h> #include <linux/platform_data/x86/pmc_atom.h>
+#include <linux/platform_device.h>
It should go after (e comes after a).
+#include <linux/platform_data/x86/clk-byt-plt.h>
struct pmc_bit_map { const char *name; @@ -36,6 +38,11 @@ struct pmc_reg_map { const struct pmc_bit_map *pss; };
+struct pmc_data {
const struct pmc_reg_map *map;const struct pmc_clk *clks;
clks -> clocks
+};
struct pmc_dev { u32 base_addr; void __iomem *regmap; @@ -49,6 +56,29 @@ struct pmc_dev { static struct pmc_dev pmc_device; static u32 acpi_base_addr;
+static const struct pmc_clk byt_clks[] = {
{.name = "xtal",.freq = 25000000,.parent_name = NULL,},{.name = "pll",.freq = 19200000,.parent_name = "xtal",},{},+};
+static const struct pmc_clk cht_clks[] = {
{.name = "xtal",.freq = 19200000,.parent_name = NULL,},{},+};
Okay, this is definition of clock trees. It means that clk-x86-pmc can be used basically on any of PMC that provides similar clock tree, right?
static const struct pmc_bit_map d3_sts_0_map[] = { {"LPSS1_F0_DMA", BIT_LPSS1_F0_DMA}, {"LPSS1_F1_PWM1", BIT_LPSS1_F1_PWM1}, @@ -168,6 +198,16 @@ struct pmc_dev { .pss = cht_pss_map, };
+static const struct pmc_data byt_data = {
.map = &byt_reg_map,.clks = byt_clks,+};
+static const struct pmc_data cht_data = {
.map = &cht_reg_map,.clks = cht_clks,+};
static inline u32 pmc_reg_read(struct pmc_dev *pmc, int reg_offset) { return readl(pmc->regmap + reg_offset); @@ -381,10 +421,36 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) } #endif /* CONFIG_DEBUG_FS */
+static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
_clks -> _clocks
const struct pmc_data *pmc_data)+{
struct platform_device *clkdev;struct pmc_clk_data *clk_data;clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
Can we just use stack for that?
if (!clk_data)return -ENOMEM;clk_data->base = pmc_regmap + PMC_CLK_CTL_0;clk_data->clks = pmc_data->clks;clkdev = platform_device_register_data(&pdev->dev, "clk-byt-plt", -1,
-1 has a definition in this case. AUTO smth... or NONE, I don't remember which one.
clk_data, sizeof(*clk_data));if (IS_ERR(clkdev)) {kfree(clk_data);return PTR_ERR(clkdev);}kfree(clk_data);return 0;+}
static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) { struct pmc_dev *pmc = &pmc_device;
const struct pmc_reg_map *map = (struct pmc_reg_map *)ent->driver_data;
const struct pmc_data *data = (struct pmc_data *)ent->driver_data;const struct pmc_reg_map *map = data->map; int ret; /* Obtain ACPI base address */@@ -413,6 +479,12 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) dev_warn(&pdev->dev, "debugfs register failed\n");
/* Register platform clocks - PMC_PLT_CLK [5:0] */ret = pmc_setup_clks(pdev, pmc->regmap, data);if (ret)dev_warn(&pdev->dev, "platform clocks register failed: %d\n",ret);pmc->init = true; return ret;} @@ -423,8 +495,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
- used by pci_match_id() call below.
*/ static const struct pci_device_id pmc_pci_ids[] = {
{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_VLV_PMC), (kernel_ulong_t)&byt_reg_map },{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_CHT_PMC), (kernel_ulong_t)&cht_reg_map },
{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_VLV_PMC), (kernel_ulong_t)&byt_data },{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_CHT_PMC), (kernel_ulong_t)&cht_data }, { 0, },};
diff --git a/include/linux/platform_data/x86/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h index aa8744c..2d310cf 100644 --- a/include/linux/platform_data/x86/pmc_atom.h +++ b/include/linux/platform_data/x86/pmc_atom.h @@ -50,6 +50,9 @@ BIT_ORED_DEDICATED_IRQ_GPSC | \ BIT_SHARED_IRQ_GPSS)
+/* Platform clock control registers */ +#define PMC_CLK_CTL_0 0x60
/* The timers acumulate time spent in sleep state */
Typo: accumulate
#define PMC_S0IR_TMR 0x80 #define PMC_S0I1_TMR 0x84
participants (6)
-
Andy Shevchenko -
Darren Hart -
Irina Tirdea -
Mark Brown -
Pierre-Louis Bossart -
Stephen Boyd