* [PATCH v6 0/3] Add platform clock for BayTrail platforms @ 2016-12-09 18:01 Irina Tirdea 2016-12-09 18:01 ` [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Irina Tirdea ` (2 more replies) 0 siblings, 3 replies; 30+ messages in thread From: Irina Tirdea @ 2016-12-09 18:01 UTC (permalink / raw) To: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner Cc: Michael Turquette, Ingo Molnar, H. Peter Anvin, alsa-devel, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, Irina Tirdea, linux-acpi, linux-kernel 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%) -- 1.9.1 ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-09 18:01 [PATCH v6 0/3] Add platform clock for BayTrail platforms Irina Tirdea @ 2016-12-09 18:01 ` Irina Tirdea 2016-12-12 23:39 ` Andy Shevchenko 2016-12-09 18:01 ` [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Irina Tirdea 2016-12-09 18:01 ` [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks Irina Tirdea 2 siblings, 1 reply; 30+ messages in thread From: Irina Tirdea @ 2016-12-09 18:01 UTC (permalink / raw) To: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner Cc: Michael Turquette, Ingo Molnar, H. Peter Anvin, alsa-devel, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, Irina Tirdea, linux-acpi, linux-kernel, Pierre-Louis Bossart 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 */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-09 18:01 ` [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Irina Tirdea @ 2016-12-12 23:39 ` Andy Shevchenko 2016-12-13 0:15 ` Pierre-Louis Bossart 2016-12-13 23:25 ` Stephen Boyd 0 siblings, 2 replies; 30+ messages in thread From: Andy Shevchenko @ 2016-12-12 23:39 UTC (permalink / raw) To: Irina Tirdea Cc: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel, Pierre-Louis Bossart 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 */ -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-12 23:39 ` Andy Shevchenko @ 2016-12-13 0:15 ` Pierre-Louis Bossart 2016-12-13 0:26 ` Andy Shevchenko 2016-12-13 1:16 ` Andy Shevchenko 2016-12-13 23:25 ` Stephen Boyd 1 sibling, 2 replies; 30+ messages in thread From: Pierre-Louis Bossart @ 2016-12-13 0:15 UTC (permalink / raw) To: Andy Shevchenko, Irina Tirdea Cc: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel > 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.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) ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-13 0:15 ` Pierre-Louis Bossart @ 2016-12-13 0:26 ` Andy Shevchenko 2016-12-16 18:36 ` Darren Hart 2016-12-13 1:16 ` Andy Shevchenko 1 sibling, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-13 0:26 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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.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) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-13 0:26 ` Andy Shevchenko @ 2016-12-16 18:36 ` Darren Hart 2016-12-16 18:49 ` Andy Shevchenko 2016-12-19 11:04 ` Mark Brown 0 siblings, 2 replies; 30+ messages in thread From: Darren Hart @ 2016-12-16 18:36 UTC (permalink / raw) To: Andy Shevchenko Cc: Pierre-Louis Bossart, Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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: > 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 > -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 18:36 ` Darren Hart @ 2016-12-16 18:49 ` Andy Shevchenko 2016-12-16 19:19 ` Darren Hart 2016-12-19 11:04 ` Mark Brown 1 sibling, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-16 18:49 UTC (permalink / raw) To: Darren Hart Cc: Pierre-Louis Bossart, Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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: > > 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... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 18:49 ` Andy Shevchenko @ 2016-12-16 19:19 ` Darren Hart 2016-12-16 22:29 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Darren Hart @ 2016-12-16 19:19 UTC (permalink / raw) To: Andy Shevchenko Cc: Pierre-Louis Bossart, Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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: > > > > 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. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 19:19 ` Darren Hart @ 2016-12-16 22:29 ` Andy Shevchenko 2016-12-16 22:58 ` Darren Hart 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-16 22:29 UTC (permalink / raw) To: Darren Hart Cc: Pierre-Louis Bossart, Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 22:29 ` Andy Shevchenko @ 2016-12-16 22:58 ` Darren Hart 0 siblings, 0 replies; 30+ messages in thread From: Darren Hart @ 2016-12-16 22:58 UTC (permalink / raw) To: Andy Shevchenko Cc: Pierre-Louis Bossart, Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 18:36 ` Darren Hart 2016-12-16 18:49 ` Andy Shevchenko @ 2016-12-19 11:04 ` Mark Brown 1 sibling, 0 replies; 30+ messages in thread From: Mark Brown @ 2016-12-19 11:04 UTC (permalink / raw) To: Darren Hart Cc: Andy Shevchenko, Pierre-Louis Bossart, Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel [-- Attachment #1: Type: text/plain, Size: 618 bytes --] 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-13 0:15 ` Pierre-Louis Bossart 2016-12-13 0:26 ` Andy Shevchenko @ 2016-12-13 1:16 ` Andy Shevchenko 1 sibling, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2016-12-13 1:16 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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.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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-12 23:39 ` Andy Shevchenko 2016-12-13 0:15 ` Pierre-Louis Bossart @ 2016-12-13 23:25 ` Stephen Boyd 2016-12-16 5:15 ` [alsa-devel] " Pierre-Louis Bossart 1 sibling, 1 reply; 30+ messages in thread From: Stephen Boyd @ 2016-12-13 23:25 UTC (permalink / raw) To: Andy Shevchenko Cc: Irina Tirdea, linux-clk, x86, platform-driver-x86, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel, Pierre-Louis Bossart 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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-13 23:25 ` Stephen Boyd @ 2016-12-16 5:15 ` Pierre-Louis Bossart 2016-12-16 8:46 ` Andy Shevchenko 2016-12-17 1:33 ` Stephen Boyd 0 siblings, 2 replies; 30+ messages in thread From: Pierre-Louis Bossart @ 2016-12-16 5:15 UTC (permalink / raw) To: Stephen Boyd, Andy Shevchenko Cc: ALSA Development Mailing List, Irina Tirdea, linux-kernel, Michael Turquette, x86, Rafael J. Wysocki, Takashi Iwai, platform-driver-x86, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Thomas Gleixner, Len Brown, linux-clk, Pierre-Louis Bossart 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. > ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 5:15 ` [alsa-devel] " Pierre-Louis Bossart @ 2016-12-16 8:46 ` Andy Shevchenko 2016-12-16 14:57 ` Pierre-Louis Bossart 2016-12-17 1:33 ` Stephen Boyd 1 sibling, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-16 8:46 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Stephen Boyd, ALSA Development Mailing List, Irina Tirdea, linux-kernel, Michael Turquette, x86, Rafael J. Wysocki, Takashi Iwai, platform-driver-x86, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Thomas Gleixner, Len Brown, linux-clk, Pierre-Louis Bossart 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 8:46 ` Andy Shevchenko @ 2016-12-16 14:57 ` Pierre-Louis Bossart 0 siblings, 0 replies; 30+ messages in thread From: Pierre-Louis Bossart @ 2016-12-16 14:57 UTC (permalink / raw) To: Andy Shevchenko Cc: Stephen Boyd, ALSA Development Mailing List, Irina Tirdea, linux-kernel, Michael Turquette, x86, Rafael J. Wysocki, Takashi Iwai, platform-driver-x86, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Thomas Gleixner, Len Brown, linux-clk, Pierre-Louis Bossart 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. > > 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-16 5:15 ` [alsa-devel] " Pierre-Louis Bossart 2016-12-16 8:46 ` Andy Shevchenko @ 2016-12-17 1:33 ` Stephen Boyd 2016-12-17 13:57 ` Andy Shevchenko 1 sibling, 1 reply; 30+ messages in thread From: Stephen Boyd @ 2016-12-17 1:33 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Andy Shevchenko, ALSA Development Mailing List, Irina Tirdea, linux-kernel, Michael Turquette, x86, Rafael J. Wysocki, Takashi Iwai, platform-driver-x86, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Thomas Gleixner, Len Brown, linux-clk, Pierre-Louis Bossart 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? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-17 1:33 ` Stephen Boyd @ 2016-12-17 13:57 ` Andy Shevchenko 2016-12-19 16:11 ` Pierre-Louis Bossart 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-17 13:57 UTC (permalink / raw) To: Stephen Boyd Cc: Pierre-Louis Bossart, ALSA Development Mailing List, Irina Tirdea, linux-kernel, Michael Turquette, x86, Rafael J. Wysocki, Takashi Iwai, platform-driver-x86, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Thomas Gleixner, Len Brown, linux-clk, Pierre-Louis Bossart 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-17 13:57 ` Andy Shevchenko @ 2016-12-19 16:11 ` Pierre-Louis Bossart 2016-12-21 23:05 ` Stephen Boyd 0 siblings, 1 reply; 30+ messages in thread From: Pierre-Louis Bossart @ 2016-12-19 16:11 UTC (permalink / raw) To: Andy Shevchenko, Stephen Boyd Cc: ALSA Development Mailing List, linux-clk, Irina Tirdea, Pierre-Louis Bossart, Michael Turquette, x86, Rafael J. Wysocki, linux-kernel, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Takashi Iwai, platform-driver-x86, Thomas Gleixner, Len Brown 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-19 16:11 ` Pierre-Louis Bossart @ 2016-12-21 23:05 ` Stephen Boyd 2016-12-22 1:07 ` Pierre-Louis Bossart 0 siblings, 1 reply; 30+ messages in thread From: Stephen Boyd @ 2016-12-21 23:05 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Andy Shevchenko, ALSA Development Mailing List, linux-clk, Irina Tirdea, Pierre-Louis Bossart, Michael Turquette, x86, Rafael J. Wysocki, linux-kernel, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Takashi Iwai, platform-driver-x86, Thomas Gleixner, Len Brown 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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-21 23:05 ` Stephen Boyd @ 2016-12-22 1:07 ` Pierre-Louis Bossart 2016-12-22 18:29 ` Stephen Boyd 2016-12-22 18:42 ` Andy Shevchenko 0 siblings, 2 replies; 30+ messages in thread From: Pierre-Louis Bossart @ 2016-12-22 1:07 UTC (permalink / raw) To: Stephen Boyd Cc: Andy Shevchenko, ALSA Development Mailing List, linux-clk, Irina Tirdea, Pierre-Louis Bossart, Michael Turquette, x86, Rafael J. Wysocki, linux-kernel, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Takashi Iwai, platform-driver-x86, Thomas Gleixner, Len Brown 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? ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-22 1:07 ` Pierre-Louis Bossart @ 2016-12-22 18:29 ` Stephen Boyd 2016-12-22 18:42 ` Andy Shevchenko 1 sibling, 0 replies; 30+ messages in thread From: Stephen Boyd @ 2016-12-22 18:29 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Andy Shevchenko, ALSA Development Mailing List, linux-clk, Irina Tirdea, Pierre-Louis Bossart, Michael Turquette, x86, Rafael J. Wysocki, linux-kernel, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Takashi Iwai, platform-driver-x86, Thomas Gleixner, Len Brown 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. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-22 1:07 ` Pierre-Louis Bossart 2016-12-22 18:29 ` Stephen Boyd @ 2016-12-22 18:42 ` Andy Shevchenko 2017-01-05 0:54 ` Pierre-Louis Bossart 1 sibling, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-22 18:42 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: Stephen Boyd, ALSA Development Mailing List, linux-clk, Irina Tirdea, Pierre-Louis Bossart, Michael Turquette, x86, Rafael J. Wysocki, linux-kernel, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Takashi Iwai, platform-driver-x86, Thomas Gleixner, Len Brown 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 ? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks 2016-12-22 18:42 ` Andy Shevchenko @ 2017-01-05 0:54 ` Pierre-Louis Bossart 0 siblings, 0 replies; 30+ messages in thread From: Pierre-Louis Bossart @ 2017-01-05 0:54 UTC (permalink / raw) To: Andy Shevchenko Cc: ALSA Development Mailing List, Len Brown, Irina Tirdea, Rafael J. Wysocki, Michael Turquette, x86, Stephen Boyd, linux-kernel, Takashi Iwai, linux-acpi, Ingo Molnar, Mark Brown, H. Peter Anvin, Darren Hart, Thomas Gleixner, platform-driver-x86, linux-clk, Pierre-Louis Bossart >>>> 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. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 2016-12-09 18:01 [PATCH v6 0/3] Add platform clock for BayTrail platforms Irina Tirdea 2016-12-09 18:01 ` [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Irina Tirdea @ 2016-12-09 18:01 ` Irina Tirdea 2016-12-12 23:43 ` Andy Shevchenko 2016-12-09 18:01 ` [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks Irina Tirdea 2 siblings, 1 reply; 30+ messages in thread From: Irina Tirdea @ 2016-12-09 18:01 UTC (permalink / raw) To: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner Cc: Michael Turquette, Ingo Molnar, H. Peter Anvin, alsa-devel, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, Irina Tirdea, linux-acpi, linux-kernel 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 -- 1.9.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 2016-12-09 18:01 ` [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Irina Tirdea @ 2016-12-12 23:43 ` Andy Shevchenko 2016-12-16 18:20 ` Darren Hart 0 siblings, 1 reply; 30+ messages in thread From: Andy Shevchenko @ 2016-12-12 23:43 UTC (permalink / raw) To: Irina Tirdea Cc: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 2016-12-12 23:43 ` Andy Shevchenko @ 2016-12-16 18:20 ` Darren Hart 2016-12-16 18:39 ` Andy Shevchenko 0 siblings, 1 reply; 30+ messages in thread From: Darren Hart @ 2016-12-16 18:20 UTC (permalink / raw) To: Andy Shevchenko Cc: Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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. -- Darren Hart Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 2016-12-16 18:20 ` Darren Hart @ 2016-12-16 18:39 ` Andy Shevchenko 0 siblings, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2016-12-16 18:39 UTC (permalink / raw) To: Darren Hart Cc: Irina Tirdea, linux-clk, x86, platform-driver-x86, Stephen Boyd, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel 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 ? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks 2016-12-09 18:01 [PATCH v6 0/3] Add platform clock for BayTrail platforms Irina Tirdea 2016-12-09 18:01 ` [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Irina Tirdea 2016-12-09 18:01 ` [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Irina Tirdea @ 2016-12-09 18:01 ` Irina Tirdea 2016-12-13 0:01 ` Andy Shevchenko 2 siblings, 1 reply; 30+ messages in thread From: Irina Tirdea @ 2016-12-09 18:01 UTC (permalink / raw) To: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner Cc: Michael Turquette, Ingo Molnar, H. Peter Anvin, alsa-devel, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, Irina Tirdea, linux-acpi, linux-kernel, Pierre-Louis Bossart 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 -- 1.9.1 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks 2016-12-09 18:01 ` [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks Irina Tirdea @ 2016-12-13 0:01 ` Andy Shevchenko 0 siblings, 0 replies; 30+ messages in thread From: Andy Shevchenko @ 2016-12-13 0:01 UTC (permalink / raw) To: Irina Tirdea Cc: linux-clk, x86, platform-driver-x86, Stephen Boyd, Darren Hart, Thomas Gleixner, Michael Turquette, Ingo Molnar, H. Peter Anvin, ALSA Development Mailing List, Mark Brown, Takashi Iwai, Pierre-Louis Bossart, Rafael J. Wysocki, Len Brown, linux-acpi, linux-kernel, Pierre-Louis Bossart 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_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> 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 -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2017-01-05 0:54 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-09 18:01 [PATCH v6 0/3] Add platform clock for BayTrail platforms Irina Tirdea 2016-12-09 18:01 ` [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Irina Tirdea 2016-12-12 23:39 ` Andy Shevchenko 2016-12-13 0:15 ` Pierre-Louis Bossart 2016-12-13 0:26 ` Andy Shevchenko 2016-12-16 18:36 ` Darren Hart 2016-12-16 18:49 ` Andy Shevchenko 2016-12-16 19:19 ` Darren Hart 2016-12-16 22:29 ` Andy Shevchenko 2016-12-16 22:58 ` Darren Hart 2016-12-19 11:04 ` Mark Brown 2016-12-13 1:16 ` Andy Shevchenko 2016-12-13 23:25 ` Stephen Boyd 2016-12-16 5:15 ` [alsa-devel] " Pierre-Louis Bossart 2016-12-16 8:46 ` Andy Shevchenko 2016-12-16 14:57 ` Pierre-Louis Bossart 2016-12-17 1:33 ` Stephen Boyd 2016-12-17 13:57 ` Andy Shevchenko 2016-12-19 16:11 ` Pierre-Louis Bossart 2016-12-21 23:05 ` Stephen Boyd 2016-12-22 1:07 ` Pierre-Louis Bossart 2016-12-22 18:29 ` Stephen Boyd 2016-12-22 18:42 ` Andy Shevchenko 2017-01-05 0:54 ` Pierre-Louis Bossart 2016-12-09 18:01 ` [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Irina Tirdea 2016-12-12 23:43 ` Andy Shevchenko 2016-12-16 18:20 ` Darren Hart 2016-12-16 18:39 ` Andy Shevchenko 2016-12-09 18:01 ` [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks Irina Tirdea 2016-12-13 0:01 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).