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
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 <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+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 <[email protected]>
+ *
+ * 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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
---
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
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <[email protected]> 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 <[email protected]>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
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 <[email protected]>");
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 <[email protected]>
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
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 <[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <[email protected]> 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 <[email protected]>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
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
> 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 <[email protected]>
>> Signed-off-by: Pierre-Louis Bossart <[email protected]>
>
> 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)
On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
<[email protected]> 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 <[email protected]>
>>> Signed-off-by: Pierre-Louis Bossart
>>> <[email protected]>
>>
>>
>> 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
On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
<[email protected]> 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
On 12/13, Andy Shevchenko wrote:
> On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <[email protected]> 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
Hi Stephen,
can you elaborate on the last comment?
thanks,
-Pierre
On 12/13/2016 05:25 PM, Stephen Boyd wrote:
>
>>> + void __iomem *base,
>>> + const char **parent_names,
>>> + int num_parents)
>>> +{
>>> + struct clk_plt *pclk;
>>> + struct clk_init_data init;
>>> + int ret;
>>> +
>>> + pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
>>> + if (!pclk)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + init.name = kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id);
>> devm_kasprintf()
> Please no.
>
>>> + init.ops = &plt_clk_ops;
>>> + init.flags = 0;
>>> + init.parent_names = parent_names;
>>> + init.num_parents = num_parents;
>>> +
>>> + pclk->hw.init = &init;
>>> + pclk->reg = base + id * PMC_CLK_CTL_SIZE;
>>> + spin_lock_init(&pclk->lock);
>>> +
>>> + ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
>>> + if (ret)
>>> + goto err_free_init;
>>> +
>>> + pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL);
>>> + if (!pclk->lookup) {
>>> + ret = -ENOMEM;
>>> + goto err_free_init;
>>> + }
>>> +
>>> + kfree(init.name);
>> devm_kfree();
> It's all local to this function, devm isn't helping anything.
> Having one kfree() would be good though. And using init.name for
> the clkdev lookup is probably wrong and should be replaced with
> something more generic along with an associated device name.
I am not sure I understand this last comment.
init.name is not a constant, it's made of the "pmc_plt_clk_" string
concatenated with an id which directly maps to which hardware clock is
registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.
>
On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
<[email protected]> 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
On 12/16/16 2:46 AM, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
> <[email protected]> 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.
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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
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
> <[email protected]> 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 <[email protected]>
> >>> Signed-off-by: Pierre-Louis Bossart
> >>> <[email protected]>
> >>
> >>
> >> 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
On Fri, Dec 16, 2016 at 8:20 PM, Darren Hart <[email protected]> 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
On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <[email protected]> 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
>> <[email protected]> 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
On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <[email protected]> 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
> >> <[email protected]> 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
On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart <[email protected]> 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 <[email protected]> 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
On Sat, Dec 17, 2016 at 12:29:41AM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart <[email protected]> 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 <[email protected]> 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
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
On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[email protected]> 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
On Fri, Dec 16, 2016 at 10:36:07AM -0800, Darren Hart wrote:
> My understanding is include/linux should be more generic, rather than platform
> specific headers. So while platform_data may refer specifically to the platform
> bus drivers, it's the closest thing we have to include/platform, which would be
> ideal. I would prefer to stick with include/platform_data because:
It's not specific to the platform bus, it's for use with the platform_data
pointer embedded in struct device that all buses can have - it's
extensively used for things like I2C and SPI for example. But really it
doesn't matter *that* much.
On 12/17/16 7:57 AM, Andy Shevchenko wrote:
> On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[email protected]> wrote:
>> On 12/15, Pierre-Louis Bossart wrote:
>
>>> Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
>>> argument.
>>
>> This is the problem. Clients should be calling clk_get() like:
>>
>> clk_get(dev, "signal name in datasheet")
>>
>> where the first argument is the device and the second argument is
>> some string that is meaningful to the device, not the system as a
>> whole. The way clkdev is intended is so that the dev argument's
>> dev_name() is combined with the con_id that matches some signale
>> name in the datasheet. This way when the same IP is put into some
>> other chip, the globally unique name doesn't need to change, just
>> the device name that's registered with the lookup. Obviously this
>> breaks down quite badly when dev_name() isn't stable. Is that
>> happening here?
>
> PMC Atom is a PCI device and thus each platform would have different
> dev_name(). Do you want to list all in each consumer if consumer wants
> to work on all of them or I missed something?
>
> So, the question is how clock getting will look like to work on
> currently both CherryTrail and BayTrail.
The name pmc_plt_clk_<n> follows the data sheet specification, where
this convention is suggested:
PLT_CLK[2:0] - Camera
PLT_CLK[3] - Audio Codec
PLT_CLK[4] -
PLT_CLK[5] - COMMs
These clocks are not internal but are made available to external
components through dedicated physical pins on the package, this external
visibility limits the scope for confusions, variations. I have not seen
any skews where these clocks and pins were changed at all.
On 12/19, Pierre-Louis Bossart wrote:
> On 12/17/16 7:57 AM, Andy Shevchenko wrote:
> >On Sat, Dec 17, 2016 at 3:33 AM, Stephen Boyd <[email protected]> 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
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 <[email protected]> wrote:
>>>> On 12/15, Pierre-Louis Bossart wrote:
>>>
>>>>> Clients use devm_clk_get() with a "pmc_plt_clk_<n>"
>>>>> argument.
>>>>
>>>> This is the problem. Clients should be calling clk_get() like:
>>>>
>>>> clk_get(dev, "signal name in datasheet")
>>>>
>>>> where the first argument is the device and the second argument is
>>>> some string that is meaningful to the device, not the system as a
>>>> whole. The way clkdev is intended is so that the dev argument's
>>>> dev_name() is combined with the con_id that matches some signale
>>>> name in the datasheet. This way when the same IP is put into some
>>>> other chip, the globally unique name doesn't need to change, just
>>>> the device name that's registered with the lookup. Obviously this
>>>> breaks down quite badly when dev_name() isn't stable. Is that
>>>> happening here?
>>>
>>> PMC Atom is a PCI device and thus each platform would have different
>>> dev_name(). Do you want to list all in each consumer if consumer wants
>>> to work on all of them or I missed something?
>>>
>>> So, the question is how clock getting will look like to work on
>>> currently both CherryTrail and BayTrail.
>>
>> The name pmc_plt_clk_<n> follows the data sheet specification, where
>> this convention is suggested:
>> PLT_CLK[2:0] - Camera
>> PLT_CLK[3] - Audio Codec
>> PLT_CLK[4] -
>> PLT_CLK[5] - COMMs
>>
>> These clocks are not internal but are made available to external
>> components through dedicated physical pins on the package, this
>> external visibility limits the scope for confusions, variations. I
>> have not seen any skews where these clocks and pins were changed at
>> all.
>
> Ok, by clkdev design if a device is passed but there isn't a
> match in the lookup table it allows it to match based solely on
> the connection id. Given that the connection id is globally
> unique this will work.
>
> Hopefully we don't have two of these devices with pmc_plt_clk_<n>
> signals in a single system though. Then having the device name
> would help differentiate between the two. And then it may make
> sense to have some sort of ACPI lookup system, similar to how we
> have lookups for clks in DT.
So in short we keep the existing solution for now and will only use the
device name if and when the pmc_plt_clk_<n> identifier is no longer
unique due to hardware changes. Did I get this right?
On 12/21/2016 05:07 PM, Pierre-Louis Bossart wrote:
> On 12/21/16 5:05 PM, Stephen Boyd wrote:
>>
>> Ok, by clkdev design if a device is passed but there isn't a
>> match in the lookup table it allows it to match based solely on
>> the connection id. Given that the connection id is globally
>> unique this will work.
>>
>> Hopefully we don't have two of these devices with pmc_plt_clk_<n>
>> signals in a single system though. Then having the device name
>> would help differentiate between the two. And then it may make
>> sense to have some sort of ACPI lookup system, similar to how we
>> have lookups for clks in DT.
>
> So in short we keep the existing solution for now and will only use
> the device name if and when the pmc_plt_clk_<n> identifier is no
> longer unique due to hardware changes. Did I get this right?
Ok.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Dec 22, 2016 at 3:07 AM, Pierre-Louis Bossart
<[email protected]> 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