This series is a set of updates following the infrastructure
rework and depends on it. It will be finally posted once
the main series is merged. For the time being I would really
appreciate feedback and/or (n)acks...
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Dmitry Eremin-Solenikov <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Mike Turquette <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Alexandre Courbot <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: Lee Jones <[email protected]>
Cc: Jean Delvare <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: [email protected]
Pawel Moll (11):
misc: vexpress-syscfg: Add udelay-based delay
power/reset: vexpress: Use udelay instead of timers
clk: versatile: Split config options for sp810 and vexpress_osc
clocksource: Sched clock source for Versatile Express
GPIO: gpio-generic: Add label to platform data
mfd: vexpress-sysreg: Add labels to gpio banks
mfd: syscon: Consider platform data a regmap config name
mfd: vexpress-sysreg: Add syscon labels as platform data
hwmon: vexpress: Use devm helper for hwmon device registration
ARM: vexpress: Simplify SMP operations for DT-powered system
ARM: vexpress: move HBI check to sysreg driver
Sudeep KarkadaNagesha (1):
ARM: vexpress: remove redundant vexpress_dt_cpus_num to get cpu count
arch/arm/mach-vexpress/core.h | 3 +-
arch/arm/mach-vexpress/platsmp.c | 187 +++++++-------------------------
arch/arm/mach-vexpress/v2m.c | 57 +---------
drivers/clk/Kconfig | 9 +-
drivers/clk/versatile/Kconfig | 26 +++++
drivers/clk/versatile/Makefile | 5 +-
drivers/clocksource/Kconfig | 9 ++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/vexpress.c | 40 +++++++
drivers/gpio/gpio-generic.c | 2 +
drivers/hwmon/vexpress.c | 100 +++++------------
drivers/mfd/syscon.c | 1 +
drivers/mfd/vexpress-sysreg.c | 45 ++++----
drivers/misc/vexpress-syscfg.c | 40 ++++++-
drivers/power/reset/vexpress-poweroff.c | 10 +-
include/linux/basic_mmio_gpio.h | 1 +
include/linux/vexpress.h | 1 -
17 files changed, 216 insertions(+), 321 deletions(-)
create mode 100644 drivers/clk/versatile/Kconfig
create mode 100644 drivers/clocksource/vexpress.c
--
1.8.3.2
Use devm_hwmon_device_register_with_groups instead of
the old-style manual attributes and hwmon device registration.
Cc: Jean Delvare <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: [email protected]
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/hwmon/vexpress.c | 100 ++++++++++++-----------------------------------
1 file changed, 26 insertions(+), 74 deletions(-)
diff --git a/drivers/hwmon/vexpress.c b/drivers/hwmon/vexpress.c
index d8a6113..7c3a973 100644
--- a/drivers/hwmon/vexpress.c
+++ b/drivers/hwmon/vexpress.c
@@ -29,15 +29,6 @@ struct vexpress_hwmon_data {
struct regmap *reg;
};
-static ssize_t vexpress_hwmon_name_show(struct device *dev,
- struct device_attribute *dev_attr, char *buffer)
-{
- const char *compatible = of_get_property(dev->of_node, "compatible",
- NULL);
-
- return sprintf(buffer, "%s\n", compatible);
-}
-
static ssize_t vexpress_hwmon_label_show(struct device *dev,
struct device_attribute *dev_attr, char *buffer)
{
@@ -84,77 +75,60 @@ static ssize_t vexpress_hwmon_u64_show(struct device *dev,
to_sensor_dev_attr(dev_attr)->index));
}
-static DEVICE_ATTR(name, S_IRUGO, vexpress_hwmon_name_show, NULL);
-
-#define VEXPRESS_HWMON_ATTRS(_name, _label_attr, _input_attr) \
-struct attribute *vexpress_hwmon_attrs_##_name[] = { \
- &dev_attr_name.attr, \
- &dev_attr_##_label_attr.attr, \
- &sensor_dev_attr_##_input_attr.dev_attr.attr, \
- NULL \
-}
+#define VEXPRESS_HWMON_ATTR_GROUPS(_name, _label_attr, _input_attr) \
+static struct attribute *vexpress_hwmon_##_name##_attrs[] = { \
+ &dev_attr_##_label_attr.attr, \
+ &sensor_dev_attr_##_input_attr.dev_attr.attr, \
+ NULL \
+}; \
+ATTRIBUTE_GROUPS(vexpress_hwmon_##_name)
#if !defined(CONFIG_REGULATOR_VEXPRESS)
static DEVICE_ATTR(in1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, vexpress_hwmon_u32_show,
NULL, 1000);
-static VEXPRESS_HWMON_ATTRS(volt, in1_label, in1_input);
-static struct attribute_group vexpress_hwmon_group_volt = {
- .attrs = vexpress_hwmon_attrs_volt,
-};
+VEXPRESS_HWMON_ATTR_GROUPS(volt, in1_label, in1_input);
#endif
static DEVICE_ATTR(curr1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, vexpress_hwmon_u32_show,
NULL, 1000);
-static VEXPRESS_HWMON_ATTRS(amp, curr1_label, curr1_input);
-static struct attribute_group vexpress_hwmon_group_amp = {
- .attrs = vexpress_hwmon_attrs_amp,
-};
+VEXPRESS_HWMON_ATTR_GROUPS(amp, curr1_label, curr1_input);
static DEVICE_ATTR(temp1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, vexpress_hwmon_u32_show,
NULL, 1000);
-static VEXPRESS_HWMON_ATTRS(temp, temp1_label, temp1_input);
-static struct attribute_group vexpress_hwmon_group_temp = {
- .attrs = vexpress_hwmon_attrs_temp,
-};
+VEXPRESS_HWMON_ATTR_GROUPS(temp, temp1_label, temp1_input);
static DEVICE_ATTR(power1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, vexpress_hwmon_u32_show,
NULL, 1);
-static VEXPRESS_HWMON_ATTRS(power, power1_label, power1_input);
-static struct attribute_group vexpress_hwmon_group_power = {
- .attrs = vexpress_hwmon_attrs_power,
-};
+VEXPRESS_HWMON_ATTR_GROUPS(power, power1_label, power1_input);
static DEVICE_ATTR(energy1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
static SENSOR_DEVICE_ATTR(energy1_input, S_IRUGO, vexpress_hwmon_u64_show,
NULL, 1);
-static VEXPRESS_HWMON_ATTRS(energy, energy1_label, energy1_input);
-static struct attribute_group vexpress_hwmon_group_energy = {
- .attrs = vexpress_hwmon_attrs_energy,
-};
+VEXPRESS_HWMON_ATTR_GROUPS(energy, energy1_label, energy1_input);
static struct of_device_id vexpress_hwmon_of_match[] = {
#if !defined(CONFIG_REGULATOR_VEXPRESS)
{
.compatible = "arm,vexpress-volt",
- .data = &vexpress_hwmon_group_volt,
+ .data = vexpress_hwmon_volt_groups,
},
#endif
{
.compatible = "arm,vexpress-amp",
- .data = &vexpress_hwmon_group_amp,
+ .data = vexpress_hwmon_amp_groups,
}, {
.compatible = "arm,vexpress-temp",
- .data = &vexpress_hwmon_group_temp,
+ .data = vexpress_hwmon_temp_groups,
}, {
.compatible = "arm,vexpress-power",
- .data = &vexpress_hwmon_group_power,
+ .data = vexpress_hwmon_power_groups,
}, {
.compatible = "arm,vexpress-energy",
- .data = &vexpress_hwmon_group_energy,
+ .data = vexpress_hwmon_energy_groups,
},
{}
};
@@ -162,9 +136,10 @@ MODULE_DEVICE_TABLE(of, vexpress_hwmon_of_match);
static int vexpress_hwmon_probe(struct platform_device *pdev)
{
- int err;
const struct of_device_id *match;
struct vexpress_hwmon_data *data;
+ const char *name;
+ const struct attribute_group **groups;
data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -176,42 +151,19 @@ static int vexpress_hwmon_probe(struct platform_device *pdev)
return -ENODEV;
data->reg = devm_regmap_init_vexpress_config(&pdev->dev);
- if (!data->reg)
- return -ENODEV;
-
- err = sysfs_create_group(&pdev->dev.kobj, match->data);
- if (err)
- goto error;
-
- data->hwmon_dev = hwmon_device_register(&pdev->dev);
- if (IS_ERR(data->hwmon_dev)) {
- err = PTR_ERR(data->hwmon_dev);
- goto error;
- }
-
- return 0;
-
-error:
- sysfs_remove_group(&pdev->dev.kobj, match->data);
- return err;
-}
-
-static int vexpress_hwmon_remove(struct platform_device *pdev)
-{
- struct vexpress_hwmon_data *data = platform_get_drvdata(pdev);
- const struct of_device_id *match;
-
- hwmon_device_unregister(data->hwmon_dev);
+ if (IS_ERR(data->reg))
+ return PTR_ERR(data->reg);
- match = of_match_device(vexpress_hwmon_of_match, &pdev->dev);
- sysfs_remove_group(&pdev->dev.kobj, match->data);
+ name = of_get_property(pdev->dev.of_node, "compatible", NULL);
+ groups = (const struct attribute_group **)match->data;
+ data->hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+ name, data, groups);
- return 0;
+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
}
static struct platform_driver vexpress_hwmon_driver = {
.probe = vexpress_hwmon_probe,
- .remove = vexpress_hwmon_remove,
.driver = {
.name = DRVNAME,
.owner = THIS_MODULE,
--
1.8.3.2
As all cores must be properly described in the Device Tree,
there is no point in getting their numbers from SCU on
A5/A9 platforms. This significantly simplifies the code,
removing the need for flat-tree scanning and early static
mapping.
Signed-off-by: Pawel Moll <[email protected]>
---
arch/arm/mach-vexpress/core.h | 3 +-
arch/arm/mach-vexpress/platsmp.c | 158 ++++++++++-----------------------------
arch/arm/mach-vexpress/v2m.c | 6 +-
3 files changed, 41 insertions(+), 126 deletions(-)
diff --git a/arch/arm/mach-vexpress/core.h b/arch/arm/mach-vexpress/core.h
index bde4374..152fad9 100644
--- a/arch/arm/mach-vexpress/core.h
+++ b/arch/arm/mach-vexpress/core.h
@@ -4,10 +4,9 @@
/* Tile's peripherals static mappings should start here */
#define V2T_PERIPH 0xf8200000
-void vexpress_dt_smp_map_io(void);
-
bool vexpress_smp_init_ops(void);
extern struct smp_operations vexpress_smp_ops;
+extern struct smp_operations vexpress_smp_dt_ops;
extern void vexpress_cpu_die(unsigned int cpu);
diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
index 12a8751..a1f3804 100644
--- a/arch/arm/mach-vexpress/platsmp.c
+++ b/arch/arm/mach-vexpress/platsmp.c
@@ -12,8 +12,7 @@
#include <linux/errno.h>
#include <linux/smp.h>
#include <linux/io.h>
-#include <linux/of.h>
-#include <linux/of_fdt.h>
+#include <linux/of_address.h>
#include <linux/vexpress.h>
#include <asm/mcpm.h>
@@ -26,125 +25,13 @@
#include "core.h"
-#if defined(CONFIG_OF)
-
-static enum {
- GENERIC_SCU,
- CORTEX_A9_SCU,
-} vexpress_dt_scu __initdata = GENERIC_SCU;
-
-static struct map_desc vexpress_dt_cortex_a9_scu_map __initdata = {
- .virtual = V2T_PERIPH,
- /* .pfn set in vexpress_dt_init_cortex_a9_scu() */
- .length = SZ_128,
- .type = MT_DEVICE,
-};
-
-static void *vexpress_dt_cortex_a9_scu_base __initdata;
-
-const static char *vexpress_dt_cortex_a9_match[] __initconst = {
- "arm,cortex-a5-scu",
- "arm,cortex-a9-scu",
- NULL
-};
-
-static int __init vexpress_dt_find_scu(unsigned long node,
- const char *uname, int depth, void *data)
-{
- if (of_flat_dt_match(node, vexpress_dt_cortex_a9_match)) {
- phys_addr_t phys_addr;
- __be32 *reg = of_get_flat_dt_prop(node, "reg", NULL);
-
- if (WARN_ON(!reg))
- return -EINVAL;
-
- phys_addr = be32_to_cpup(reg);
- vexpress_dt_scu = CORTEX_A9_SCU;
-
- vexpress_dt_cortex_a9_scu_map.pfn = __phys_to_pfn(phys_addr);
- iotable_init(&vexpress_dt_cortex_a9_scu_map, 1);
- vexpress_dt_cortex_a9_scu_base = ioremap(phys_addr, SZ_256);
- if (WARN_ON(!vexpress_dt_cortex_a9_scu_base))
- return -EFAULT;
- }
-
- return 0;
-}
-
-void __init vexpress_dt_smp_map_io(void)
-{
- if (initial_boot_params)
- WARN_ON(of_scan_flat_dt(vexpress_dt_find_scu, NULL));
-}
-
-static void __init vexpress_dt_smp_init_cpus(void)
-{
- int ncores = 0, i;
-
- switch (vexpress_dt_scu) {
- case GENERIC_SCU:
- return;
- case CORTEX_A9_SCU:
- ncores = scu_get_core_count(vexpress_dt_cortex_a9_scu_base);
- break;
- default:
- WARN_ON(1);
- break;
- }
-
- if (ncores < 2)
- return;
-
- if (ncores > nr_cpu_ids) {
- pr_warn("SMP: %u cores greater than maximum (%u), clipping\n",
- ncores, nr_cpu_ids);
- ncores = nr_cpu_ids;
- }
-
- for (i = 0; i < ncores; ++i)
- set_cpu_possible(i, true);
-}
-
-static void __init vexpress_dt_smp_prepare_cpus(unsigned int max_cpus)
-{
-
- switch (vexpress_dt_scu) {
- case GENERIC_SCU:
- break;
- case CORTEX_A9_SCU:
- scu_enable(vexpress_dt_cortex_a9_scu_base);
- break;
- default:
- WARN_ON(1);
- break;
- }
-}
-
-#else
-
-static void __init vexpress_dt_smp_init_cpus(void)
-{
- WARN_ON(1);
-}
-
-void __init vexpress_dt_smp_prepare_cpus(unsigned int max_cpus)
-{
- WARN_ON(1);
-}
-
-#endif
-
/*
* Initialise the CPU possible map early - this describes the CPUs
* which may be present or become present in the system.
*/
static void __init vexpress_smp_init_cpus(void)
{
- if (ct_desc)
- ct_desc->init_cpu_map();
- else
- vexpress_dt_smp_init_cpus();
-
+ ct_desc->init_cpu_map();
}
static void __init vexpress_smp_prepare_cpus(unsigned int max_cpus)
@@ -153,10 +40,7 @@ static void __init vexpress_smp_prepare_cpus(unsigned int max_cpus)
* Initialise the present map, which describes the set of CPUs
* actually populated at the present time.
*/
- if (ct_desc)
- ct_desc->smp_enable(max_cpus);
- else
- vexpress_dt_smp_prepare_cpus(max_cpus);
+ ct_desc->smp_enable(max_cpus);
/*
* Write the address of secondary startup into the
@@ -194,3 +78,39 @@ bool __init vexpress_smp_init_ops(void)
#endif
return false;
}
+
+#if defined(CONFIG_OF)
+
+static const struct of_device_id vexpress_smp_dt_scu_match[] __initconst = {
+ { .compatible = "arm,cortex-a5-scu", },
+ { .compatible = "arm,cortex-a9-scu", },
+ {}
+};
+
+static void __init vexpress_smp_dt_prepare_cpus(unsigned int max_cpus)
+{
+ struct device_node *scu = of_find_matching_node(NULL,
+ vexpress_smp_dt_scu_match);
+
+ if (scu)
+ scu_enable(of_iomap(scu, 0));
+
+ /*
+ * Write the address of secondary startup into the
+ * system-wide flags register. The boot monitor waits
+ * until it receives a soft interrupt, and then the
+ * secondary CPU branches to this address.
+ */
+ vexpress_flags_set(virt_to_phys(versatile_secondary_startup));
+}
+
+struct smp_operations __initdata vexpress_smp_dt_ops = {
+ .smp_prepare_cpus = vexpress_smp_dt_prepare_cpus,
+ .smp_secondary_init = versatile_secondary_init,
+ .smp_boot_secondary = versatile_boot_secondary,
+#ifdef CONFIG_HOTPLUG_CPU
+ .cpu_die = vexpress_cpu_die,
+#endif
+};
+
+#endif
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index d8a9fd7..d8b419b 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -400,10 +400,6 @@ void __init v2m_dt_map_io(void)
iotable_init(&v2m_rs1_io_desc, 1);
else
iotable_init(v2m_io_desc, ARRAY_SIZE(v2m_io_desc));
-
-#if defined(CONFIG_SMP)
- vexpress_dt_smp_map_io();
-#endif
}
void __init v2m_dt_init_early(void)
@@ -434,7 +430,7 @@ static const char * const v2m_dt_match[] __initconst = {
DT_MACHINE_START(VEXPRESS_DT, "ARM-Versatile Express")
.dt_compat = v2m_dt_match,
- .smp = smp_ops(vexpress_smp_ops),
+ .smp = smp_ops(vexpress_smp_dt_ops),
.smp_init = smp_init_ops(vexpress_smp_init_ops),
.map_io = v2m_dt_map_io,
.init_early = v2m_dt_init_early,
--
1.8.3.2
The last reason for static memory mapping is the HBI (board
identification number) check early in the machine code.
Moving the check to the sysreg driver makes it possible to
completely remove the early mapping and init functions.
Signed-off-by: Pawel Moll <[email protected]>
---
arch/arm/mach-vexpress/v2m.c | 49 -------------------------------------------
drivers/mfd/vexpress-sysreg.c | 30 ++++++++++----------------
include/linux/vexpress.h | 1 -
3 files changed, 11 insertions(+), 69 deletions(-)
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index d8b419b..38f4f6f 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -370,53 +370,6 @@ MACHINE_START(VEXPRESS, "ARM-Versatile Express")
.init_machine = v2m_init,
MACHINE_END
-static struct map_desc v2m_rs1_io_desc __initdata = {
- .virtual = V2M_PERIPH,
- .pfn = __phys_to_pfn(0x1c000000),
- .length = SZ_2M,
- .type = MT_DEVICE,
-};
-
-static int __init v2m_dt_scan_memory_map(unsigned long node, const char *uname,
- int depth, void *data)
-{
- const char **map = data;
-
- if (strcmp(uname, "motherboard") != 0)
- return 0;
-
- *map = of_get_flat_dt_prop(node, "arm,v2m-memory-map", NULL);
-
- return 1;
-}
-
-void __init v2m_dt_map_io(void)
-{
- const char *map = NULL;
-
- of_scan_flat_dt(v2m_dt_scan_memory_map, &map);
-
- if (map && strcmp(map, "rs1") == 0)
- iotable_init(&v2m_rs1_io_desc, 1);
- else
- iotable_init(v2m_io_desc, ARRAY_SIZE(v2m_io_desc));
-}
-
-void __init v2m_dt_init_early(void)
-{
- u32 dt_hbi;
-
- /* Confirm board type against DT property, if available */
- if (of_property_read_u32(of_allnodes, "arm,hbi", &dt_hbi) == 0) {
- u32 hbi = vexpress_get_hbi(VEXPRESS_SITE_MASTER);
-
- if (WARN_ON(dt_hbi != hbi))
- pr_warning("vexpress: DT HBI (%x) is not matching "
- "hardware (%x)!\n", dt_hbi, hbi);
- }
-}
-
-
static void __init v2m_dt_init(void)
{
l2x0_of_init(0x00400000, 0xfe0fffff);
@@ -432,7 +385,5 @@ DT_MACHINE_START(VEXPRESS_DT, "ARM-Versatile Express")
.dt_compat = v2m_dt_match,
.smp = smp_ops(vexpress_smp_dt_ops),
.smp_init = smp_init_ops(vexpress_smp_init_ops),
- .map_io = v2m_dt_map_io,
- .init_early = v2m_dt_init_early,
.init_machine = v2m_dt_init,
MACHINE_END
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index 57c21a0..3eb26e1 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -44,7 +44,6 @@
#define SYS_CFGSTAT 0x0a8
#define SYS_HBI_MASK 0xfff
-#define SYS_ID_HBI_SHIFT 16
#define SYS_PROCIDx_HBI_SHIFT 0
#define SYS_MCI_CARDIN (1 << 0)
@@ -98,24 +97,6 @@ u32 vexpress_get_procid(int site)
SYS_PROCID0 : SYS_PROCID1));
}
-u32 vexpress_get_hbi(int site)
-{
- u32 id;
-
- switch (site) {
- case VEXPRESS_SITE_MB:
- id = readl(vexpress_sysreg_base() + SYS_ID);
- return (id >> SYS_ID_HBI_SHIFT) & SYS_HBI_MASK;
- case VEXPRESS_SITE_MASTER:
- case VEXPRESS_SITE_DB1:
- case VEXPRESS_SITE_DB2:
- id = vexpress_get_procid(site);
- return (id >> SYS_PROCIDx_HBI_SHIFT) & SYS_HBI_MASK;
- }
-
- return ~0;
-}
-
void __iomem *vexpress_get_24mhz_clock_base(void)
{
return vexpress_sysreg_base() + SYS_24MHZ;
@@ -222,6 +203,7 @@ static int vexpress_sysreg_probe(struct platform_device *pdev)
struct resource *mem;
void __iomem *base;
struct bgpio_chip *mmc_gpio_chip;
+ u32 dt_hbi;
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!mem)
@@ -233,6 +215,16 @@ static int vexpress_sysreg_probe(struct platform_device *pdev)
vexpress_config_set_master(vexpress_sysreg_get_master());
+ /* Confirm board type against DT property, if available */
+ if (of_property_read_u32(of_allnodes, "arm,hbi", &dt_hbi) == 0) {
+ u32 id = vexpress_get_procid(VEXPRESS_SITE_MASTER);
+ u32 hbi = (id >> SYS_PROCIDx_HBI_SHIFT) & SYS_HBI_MASK;
+
+ if (WARN_ON(dt_hbi != hbi))
+ dev_warn(&pdev->dev, "DT HBI (%x) is not matching hardware (%x)!\n",
+ dt_hbi, hbi);
+ }
+
/*
* Duplicated SYS_MCI pseudo-GPIO controller for compatibility with
* older trees using sysreg node for MMC control lines.
diff --git a/include/linux/vexpress.h b/include/linux/vexpress.h
index 0aee1b9..06e1ab8 100644
--- a/include/linux/vexpress.h
+++ b/include/linux/vexpress.h
@@ -60,7 +60,6 @@ struct regmap *devm_regmap_init_vexpress_config(struct device *dev);
unsigned int vexpress_get_mci_cardin(struct device *dev);
u32 vexpress_get_procid(int site);
-u32 vexpress_get_hbi(int site);
void *vexpress_get_24mhz_clock_base(void);
void vexpress_flags_set(u32 data);
--
1.8.3.2
From: Sudeep KarkadaNagesha <[email protected]>
arm_dt_init_cpu_maps parses the device tree, validates and sets the
cpu_possible_mask appropriately. It is unnecessary to do another DT
parse to get the number of cpus, use num_possible_cpus instead.
This patch also removes setting cpu_present_mask as platforms should
only re-initialize it in smp_prepare_cpus() if present != possible.
Signed-off-by: Sudeep KarkadaNagesha <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
arch/arm/mach-vexpress/platsmp.c | 31 +------------------------------
1 file changed, 1 insertion(+), 30 deletions(-)
diff --git a/arch/arm/mach-vexpress/platsmp.c b/arch/arm/mach-vexpress/platsmp.c
index 993c9ae..12a8751 100644
--- a/arch/arm/mach-vexpress/platsmp.c
+++ b/arch/arm/mach-vexpress/platsmp.c
@@ -77,39 +77,13 @@ void __init vexpress_dt_smp_map_io(void)
WARN_ON(of_scan_flat_dt(vexpress_dt_find_scu, NULL));
}
-static int __init vexpress_dt_cpus_num(unsigned long node, const char *uname,
- int depth, void *data)
-{
- static int prev_depth = -1;
- static int nr_cpus = -1;
-
- if (prev_depth > depth && nr_cpus > 0)
- return nr_cpus;
-
- if (nr_cpus < 0 && strcmp(uname, "cpus") == 0)
- nr_cpus = 0;
-
- if (nr_cpus >= 0) {
- const char *device_type = of_get_flat_dt_prop(node,
- "device_type", NULL);
-
- if (device_type && strcmp(device_type, "cpu") == 0)
- nr_cpus++;
- }
-
- prev_depth = depth;
-
- return 0;
-}
-
static void __init vexpress_dt_smp_init_cpus(void)
{
int ncores = 0, i;
switch (vexpress_dt_scu) {
case GENERIC_SCU:
- ncores = of_scan_flat_dt(vexpress_dt_cpus_num, NULL);
- break;
+ return;
case CORTEX_A9_SCU:
ncores = scu_get_core_count(vexpress_dt_cortex_a9_scu_base);
break;
@@ -133,12 +107,9 @@ static void __init vexpress_dt_smp_init_cpus(void)
static void __init vexpress_dt_smp_prepare_cpus(unsigned int max_cpus)
{
- int i;
switch (vexpress_dt_scu) {
case GENERIC_SCU:
- for (i = 0; i < max_cpus; i++)
- set_cpu_present(i, true);
break;
case CORTEX_A9_SCU:
scu_enable(vexpress_dt_cortex_a9_scu_base);
--
1.8.3.2
... so debugfs interfaces are easier to use.
Cc: Linus Walleij <[email protected]>
Cc: Alexandre Courbot <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: Lee Jones <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/mfd/vexpress-sysreg.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index f639dff..ece5ac8 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -133,16 +133,19 @@ void __init vexpress_sysreg_early_init(void __iomem *base)
/* The sysreg block is just a random collection of various functions... */
static struct bgpio_pdata vexpress_sysreg_sys_led_pdata = {
+ .label = "sys_led",
.base = -1,
.ngpio = 8,
};
static struct bgpio_pdata vexpress_sysreg_sys_mci_pdata = {
+ .label = "sys_mci",
.base = -1,
.ngpio = 2,
};
static struct bgpio_pdata vexpress_sysreg_sys_flash_pdata = {
+ .label = "sys_flash",
.base = -1,
.ngpio = 1,
};
--
1.8.3.2
This patch adds label names for syscon registers as platform
data for the relevant MFD cells.
Cc: Samuel Ortiz <[email protected]>
Cc: Lee Jones <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/mfd/vexpress-sysreg.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index ece5ac8..57c21a0 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -132,6 +132,8 @@ void __init vexpress_sysreg_early_init(void __iomem *base)
/* The sysreg block is just a random collection of various functions... */
+static const char vexpress_sysreg_sys_id_pdata[] = "sys_id";
+
static struct bgpio_pdata vexpress_sysreg_sys_led_pdata = {
.label = "sys_led",
.base = -1,
@@ -150,6 +152,10 @@ static struct bgpio_pdata vexpress_sysreg_sys_flash_pdata = {
.ngpio = 1,
};
+static const char vexpress_sysreg_sys_misc_pdata[] = "sys_misc";
+
+static const char vexpress_sysreg_sys_procid_pdata[] = "sys_procid";
+
static struct mfd_cell vexpress_sysreg_cells[] = {
{
.name = "syscon",
@@ -157,6 +163,8 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
.resources = (struct resource []) {
DEFINE_RES_MEM(SYS_ID, 0x4),
},
+ .platform_data = &vexpress_sysreg_sys_id_pdata,
+ .pdata_size = sizeof(vexpress_sysreg_sys_id_pdata),
}, {
.name = "basic-mmio-gpio",
.of_compatible = "arm,vexpress-sysreg,sys_led",
@@ -190,12 +198,16 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
.resources = (struct resource []) {
DEFINE_RES_MEM(SYS_MISC, 0x4),
},
+ .platform_data = &vexpress_sysreg_sys_misc_pdata,
+ .pdata_size = sizeof(vexpress_sysreg_sys_misc_pdata),
}, {
.name = "syscon",
.num_resources = 1,
.resources = (struct resource []) {
DEFINE_RES_MEM(SYS_PROCID0, 0x8),
},
+ .platform_data = &vexpress_sysreg_sys_procid_pdata,
+ .pdata_size = sizeof(vexpress_sysreg_sys_procid_pdata),
}, {
.name = "vexpress-syscfg",
.num_resources = 1,
--
1.8.3.2
Use the device platform data as a regmap config
name. This is particularly useful in the regmap
debugfs when there is more than one syscon device
registered, to distinguish the register blocks.
Cc: Samuel Ortiz <[email protected]>
Cc: Lee Jones <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
Alternatively I could define a syscon platform data structure,
something like this:
struct syscon_platform_data {
const char *label;
};
drivers/mfd/syscon.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 71841f9..ea1770b 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -143,6 +143,7 @@ static int syscon_probe(struct platform_device *pdev)
return -ENOMEM;
syscon_regmap_config.max_register = res->end - res->start - 3;
+ syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
syscon->regmap = devm_regmap_init_mmio(dev, base,
&syscon_regmap_config);
if (IS_ERR(syscon->regmap)) {
--
1.8.3.2
It is normally preferable to yield the task
waiting for syscfg operations (that can take
up to dozens of milliseconds), but when the
system is shutting down it may not be possible.
This patch adds a udelay-based version of the
code to be used in such circumstances.
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/misc/vexpress-syscfg.c | 40 ++++++++++++++++++++++++++++++++++++----
1 file changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/vexpress-syscfg.c b/drivers/misc/vexpress-syscfg.c
index 4d661ef..82f8229 100644
--- a/drivers/misc/vexpress-syscfg.c
+++ b/drivers/misc/vexpress-syscfg.c
@@ -53,6 +53,37 @@ struct vexpress_syscfg_func {
};
+static int vexpress_syscfg_delay_schedule(unsigned int us)
+{
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(usecs_to_jiffies(us));
+ if (signal_pending(current))
+ return -EINTR;
+
+ return 0;
+}
+
+static int vexpress_syscfg_delay_loop(unsigned int us)
+{
+ udelay(us);
+
+ return 0;
+}
+
+static int (*vexpress_syscfg_delay)(unsigned int us) =
+ vexpress_syscfg_delay_schedule;
+
+static void vexpress_syscfg_shutdown(void)
+{
+ /* Can't relay on the scheduler when the system is going down */
+ vexpress_syscfg_delay = vexpress_syscfg_delay_loop;
+}
+
+static struct syscore_ops vexpress_syscfg_syscore_ops = {
+ .shutdown = vexpress_syscfg_shutdown,
+};
+
+
static int vexpress_syscfg_exec(struct vexpress_syscfg_func *func,
int index, bool write, u32 *data)
{
@@ -87,10 +118,9 @@ static int vexpress_syscfg_exec(struct vexpress_syscfg_func *func,
tries = 100;
timeout = 100;
do {
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(usecs_to_jiffies(timeout));
- if (signal_pending(current))
- return -EINTR;
+ int err = vexpress_syscfg_delay(timeout);
+ if (err)
+ return err;
status = readl(syscfg->base + SYS_CFGSTAT);
if (status & SYS_CFGSTAT_ERR)
@@ -299,6 +329,8 @@ int vexpress_syscfg_probe(struct platform_device *pdev)
if (!pdev->dev.of_node)
vexpress_syscfg_bridge = bridge;
+ register_syscore_ops(&vexpress_syscfg_syscore_ops);
+
return 0;
}
--
1.8.3.2
When registering more than one platform device, it is
useful to set the gpio chip label in the platform data.
Cc: Linus Walleij <[email protected]>
Cc: Alexandre Courbot <[email protected]>
Cc: Samuel Ortiz <[email protected]>
Cc: Lee Jones <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
This patch has been already merged by Linux; it has been included
for completeness only.
drivers/gpio/gpio-generic.c | 2 ++
include/linux/basic_mmio_gpio.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index d2196bf..8c778af 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -531,6 +531,8 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
return err;
if (pdata) {
+ if (pdata->label)
+ bgc->gc.label = pdata->label;
bgc->gc.base = pdata->base;
if (pdata->ngpio > 0)
bgc->gc.ngpio = pdata->ngpio;
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index d8a97ec..0e97856 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -19,6 +19,7 @@
#include <linux/spinlock_types.h>
struct bgpio_pdata {
+ const char *label;
int base;
int ngpio;
};
--
1.8.3.2
This patch adds a trival sched clock source using free
running, 24MHz clocked counter present in the ARM Ltd.
Versatile Express platform's System Registers block.
This code replaces the call in the VE machine code.
Cc: Daniel Lezcano <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
arch/arm/mach-vexpress/v2m.c | 2 --
drivers/clocksource/Kconfig | 9 +++++++++
drivers/clocksource/Makefile | 1 +
drivers/clocksource/vexpress.c | 40 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 50 insertions(+), 2 deletions(-)
create mode 100644 drivers/clocksource/vexpress.c
diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
index 90f04c9..d8a9fd7 100644
--- a/arch/arm/mach-vexpress/v2m.c
+++ b/arch/arm/mach-vexpress/v2m.c
@@ -418,8 +418,6 @@ void __init v2m_dt_init_early(void)
pr_warning("vexpress: DT HBI (%x) is not matching "
"hardware (%x)!\n", dt_hbi, hbi);
}
-
- versatile_sched_clock_init(vexpress_get_24mhz_clock_base(), 24000000);
}
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cd6950f..9799744 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -140,3 +140,12 @@ config VF_PIT_TIMER
bool
help
Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
+
+config CLKSRC_VEXPRESS
+ bool
+ depends on MFD_VEXPRESS_SYSREG
+ depends on GENERIC_SCHED_CLOCK
+ select CLKSRC_OF
+ default y
+ help
+ Simple provider of sched clock on ARM Versatile Express platform.
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index c7ca50a..1051a23 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o
obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o
obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST) += dummy_timer.o
+obj-$(CONFIG_CLKSRC_VEXPRESS) += vexpress.o
diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
new file mode 100644
index 0000000..55b8ab4
--- /dev/null
+++ b/drivers/clocksource/vexpress.c
@@ -0,0 +1,40 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that 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.
+ *
+ * Copyright (C) 2013 ARM Limited
+ */
+
+#include <linux/clocksource.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/sched_clock.h>
+
+#define SYS_24MHZ 0x05c
+
+static void __iomem *vexpress_sys_24mhz;
+
+static u32 notrace vexpress_sys_24mhz_read(void)
+{
+ return readl(vexpress_sys_24mhz);
+}
+
+static void __init vexpress_sched_clock_init(struct device_node *node)
+{
+ void __iomem *base = of_iomap(node, 0);
+
+ if (!base)
+ return;
+
+ vexpress_sys_24mhz = base + SYS_24MHZ;
+
+ setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
+}
+CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
+ vexpress_sched_clock_init);
--
1.8.3.2
At this stage of system shutdown procedure the jiffies may
not be updated anymore, so just use udelay instead.
Cc: Dmitry Eremin-Solenikov <[email protected]>
Cc: David Woodhouse <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/power/reset/vexpress-poweroff.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c
index 37e7799..195235c 100644
--- a/drivers/power/reset/vexpress-poweroff.c
+++ b/drivers/power/reset/vexpress-poweroff.c
@@ -11,7 +11,7 @@
* Copyright (C) 2012 ARM Limited
*/
-#include <linux/jiffies.h>
+#include <linux/delay.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
@@ -26,13 +26,9 @@ static void vexpress_reset_do(struct device *dev, const char *what)
struct regmap *reg = dev_get_drvdata(dev);
if (reg) {
- unsigned long timeout;
-
err = regmap_write(reg, 0, 0);
-
- timeout = jiffies + HZ;
- while (time_before(jiffies, timeout))
- cpu_relax();
+ if (!err)
+ mdelay(1000);
}
dev_emerg(dev, "Unable to %s (%d)\n", what, err);
--
1.8.3.2
Move the Kconfig entry for Versatile (& Express) clock drivers
into a separate file and add individual options for sp810
and vexpress_osc drivers, as they are optional in some
configurations and may have separate dependencies.
Cc: Mike Turquette <[email protected]>
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/clk/Kconfig | 9 +--------
drivers/clk/versatile/Kconfig | 26 ++++++++++++++++++++++++++
drivers/clk/versatile/Makefile | 5 +++--
3 files changed, 30 insertions(+), 10 deletions(-)
create mode 100644 drivers/clk/versatile/Kconfig
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 7641965..11e7058 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -30,14 +30,7 @@ config COMMON_CLK_WM831X
Supports the clocking subsystem of the WM831x/2x series of
PMICs from Wolfson Microlectronics.
-config COMMON_CLK_VERSATILE
- bool "Clock driver for ARM Reference designs"
- depends on ARCH_INTEGRATOR || ARCH_REALVIEW || ARCH_VEXPRESS || ARM64
- ---help---
- Supports clocking on ARM Reference designs:
- - Integrator/AP and Integrator/CP
- - RealView PB1176, EB, PB11MP and PBX
- - Versatile Express
+source "drivers/clk/versatile/Kconfig"
config COMMON_CLK_MAX77686
tristate "Clock driver for Maxim 77686 MFD"
diff --git a/drivers/clk/versatile/Kconfig b/drivers/clk/versatile/Kconfig
new file mode 100644
index 0000000..1530c93
--- /dev/null
+++ b/drivers/clk/versatile/Kconfig
@@ -0,0 +1,26 @@
+config COMMON_CLK_VERSATILE
+ bool "Clock driver for ARM Reference designs"
+ depends on ARCH_INTEGRATOR || ARCH_REALVIEW || ARCH_VEXPRESS || ARM64
+ ---help---
+ Supports clocking on ARM Reference designs:
+ - Integrator/AP and Integrator/CP
+ - RealView PB1176, EB, PB11MP and PBX
+ - Versatile Express
+
+config CLK_SP810
+ bool "Clock driver for ARM SP810 System Controller"
+ depends on COMMON_CLK_VERSATILE
+ default y if ARCH_VEXPRESS
+ ---help---
+ Supports clock muxing (REFCLK/TIMCLK to TIMERCLKEN0-3) capabilities
+ of the ARM SP810 System Controller cell.
+
+config CLK_VEXPRESS_OSC
+ bool "Clock driver for Versatile Express OSC clock generators"
+ depends on COMMON_CLK_VERSATILE
+ depends on VEXPRESS_CONFIG
+ default y if ARCH_VEXPRESS
+ ---help---
+ Simple regmap-based driver driving clock generators on Versatile
+ Express platforms hidden behind its configuration infrastructure,
+ commonly known as OSCs.
diff --git a/drivers/clk/versatile/Makefile b/drivers/clk/versatile/Makefile
index c16ca78..fd449f9 100644
--- a/drivers/clk/versatile/Makefile
+++ b/drivers/clk/versatile/Makefile
@@ -3,5 +3,6 @@ obj-$(CONFIG_ICST) += clk-icst.o
obj-$(CONFIG_ARCH_INTEGRATOR) += clk-integrator.o
obj-$(CONFIG_INTEGRATOR_IMPD1) += clk-impd1.o
obj-$(CONFIG_ARCH_REALVIEW) += clk-realview.o
-obj-$(CONFIG_ARCH_VEXPRESS) += clk-vexpress.o clk-sp810.o
-obj-$(CONFIG_VEXPRESS_CONFIG) += clk-vexpress-osc.o
+obj-$(CONFIG_ARCH_VEXPRESS) += clk-vexpress.o
+obj-$(CONFIG_CLK_SP810) += clk-sp810.o
+obj-$(CONFIG_CLK_VEXPRESS_OSC) += clk-vexpress-osc.o
--
1.8.3.2
> When registering more than one platform device, it is
> useful to set the gpio chip label in the platform data.
>
> Cc: Linus Walleij <[email protected]>
> Cc: Alexandre Courbot <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> This patch has been already merged by Linux; it has been included
> for completeness only.
>
> drivers/gpio/gpio-generic.c | 2 ++
> include/linux/basic_mmio_gpio.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> index d2196bf..8c778af 100644
> --- a/drivers/gpio/gpio-generic.c
> +++ b/drivers/gpio/gpio-generic.c
> @@ -531,6 +531,8 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
> return err;
>
> if (pdata) {
> + if (pdata->label)
Why is this check necessary?
> + bgc->gc.label = pdata->label;
> bgc->gc.base = pdata->base;
> if (pdata->ngpio > 0)
> bgc->gc.ngpio = pdata->ngpio;
> diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
> index d8a97ec..0e97856 100644
> --- a/include/linux/basic_mmio_gpio.h
> +++ b/include/linux/basic_mmio_gpio.h
> @@ -19,6 +19,7 @@
> #include <linux/spinlock_types.h>
>
> struct bgpio_pdata {
> + const char *label;
> int base;
> int ngpio;
> };
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> ... so debugfs interfaces are easier to use.
>
> Cc: Linus Walleij <[email protected]>
> Cc: Alexandre Courbot <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> drivers/mfd/vexpress-sysreg.c | 3 +++
> 1 file changed, 3 insertions(+)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 2014-02-11 at 17:17 +0000, Lee Jones wrote:
> > When registering more than one platform device, it is
> > useful to set the gpio chip label in the platform data.
> >
> > Cc: Linus Walleij <[email protected]>
> > Cc: Alexandre Courbot <[email protected]>
> > Cc: Samuel Ortiz <[email protected]>
> > Cc: Lee Jones <[email protected]>
> > Signed-off-by: Pawel Moll <[email protected]>
> > ---
> > This patch has been already merged by Linux; it has been included
> > for completeness only.
> >
> > drivers/gpio/gpio-generic.c | 2 ++
> > include/linux/basic_mmio_gpio.h | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> > index d2196bf..8c778af 100644
> > --- a/drivers/gpio/gpio-generic.c
> > +++ b/drivers/gpio/gpio-generic.c
> > @@ -531,6 +531,8 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
> > return err;
> >
> > if (pdata) {
> > + if (pdata->label)
>
> Why is this check necessary?
>
> > + bgc->gc.label = pdata->label;
Because bgc->gc.label is initialized to a default value in bgpio_init()
and I don't want to override it with NULL if there is no label passed in
the platform data.
Pawel
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> Use the device platform data as a regmap config
> name. This is particularly useful in the regmap
> debugfs when there is more than one syscon device
> registered, to distinguish the register blocks.
>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> Alternatively I could define a syscon platform data structure,
> something like this:
>
> struct syscon_platform_data {
> const char *label;
> };
>
> drivers/mfd/syscon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 71841f9..ea1770b 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -143,6 +143,7 @@ static int syscon_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> syscon_regmap_config.max_register = res->end - res->start - 3;
> + syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
Perhaps the answer is waiting for me in the other set, but ...
Isn't this going to be NULL most of the time?
> syscon->regmap = devm_regmap_init_mmio(dev, base,
> &syscon_regmap_config);
> if (IS_ERR(syscon->regmap)) {
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> This patch adds label names for syscon registers as platform
> data for the relevant MFD cells.
Okay, I see.
Hmm... not sure I like this at all. It seems awfully as though you're
bending current infrastructure to suit your needs. There must be better
ways of passing a name than through pdata, and if there isn't I'd
suggest setting up a sysconf pdata struct that others can use in case
they wish to expand the functionality further in some other
unfathomable way. :)
> Cc: Samuel Ortiz <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> drivers/mfd/vexpress-sysreg.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
> index ece5ac8..57c21a0 100644
> --- a/drivers/mfd/vexpress-sysreg.c
> +++ b/drivers/mfd/vexpress-sysreg.c
> @@ -132,6 +132,8 @@ void __init vexpress_sysreg_early_init(void __iomem *base)
>
> /* The sysreg block is just a random collection of various functions... */
>
> +static const char vexpress_sysreg_sys_id_pdata[] = "sys_id";
> +
> static struct bgpio_pdata vexpress_sysreg_sys_led_pdata = {
> .label = "sys_led",
> .base = -1,
> @@ -150,6 +152,10 @@ static struct bgpio_pdata vexpress_sysreg_sys_flash_pdata = {
> .ngpio = 1,
> };
>
> +static const char vexpress_sysreg_sys_misc_pdata[] = "sys_misc";
> +
> +static const char vexpress_sysreg_sys_procid_pdata[] = "sys_procid";
> +
> static struct mfd_cell vexpress_sysreg_cells[] = {
> {
> .name = "syscon",
> @@ -157,6 +163,8 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
> .resources = (struct resource []) {
> DEFINE_RES_MEM(SYS_ID, 0x4),
> },
> + .platform_data = &vexpress_sysreg_sys_id_pdata,
> + .pdata_size = sizeof(vexpress_sysreg_sys_id_pdata),
> }, {
> .name = "basic-mmio-gpio",
> .of_compatible = "arm,vexpress-sysreg,sys_led",
> @@ -190,12 +198,16 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
> .resources = (struct resource []) {
> DEFINE_RES_MEM(SYS_MISC, 0x4),
> },
> + .platform_data = &vexpress_sysreg_sys_misc_pdata,
> + .pdata_size = sizeof(vexpress_sysreg_sys_misc_pdata),
> }, {
> .name = "syscon",
> .num_resources = 1,
> .resources = (struct resource []) {
> DEFINE_RES_MEM(SYS_PROCID0, 0x8),
> },
> + .platform_data = &vexpress_sysreg_sys_procid_pdata,
> + .pdata_size = sizeof(vexpress_sysreg_sys_procid_pdata),
> }, {
> .name = "vexpress-syscfg",
> .num_resources = 1,
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 2014-02-11 at 17:20 +0000, Pawel Moll wrote:
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
Uh, sorry about this. My finger slipped...
Pawel
On Tue, 2014-02-11 at 17:29 +0000, Lee Jones wrote:
> There must be better
> ways of passing a name than through pdata, and if there isn't I'd
> suggest setting up a sysconf pdata struct that others can use in case
> they wish to expand the functionality further in some other
> unfathomable way. :)
... which I proposed in the previous patch :-)
On Tue, 2014-02-11 at 17:10 +0000, Pawel Moll wrote:
> Alternatively I could define a syscon platform data structure,
> something like this:
>
> struct syscon_platform_data {
> const char *label;
> };
Pawel
> > > When registering more than one platform device, it is
> > > useful to set the gpio chip label in the platform data.
> > >
> > > Cc: Linus Walleij <[email protected]>
> > > Cc: Alexandre Courbot <[email protected]>
> > > Cc: Samuel Ortiz <[email protected]>
> > > Cc: Lee Jones <[email protected]>
> > > Signed-off-by: Pawel Moll <[email protected]>
> > > ---
> > > This patch has been already merged by Linux; it has been included
> > > for completeness only.
> > >
> > > drivers/gpio/gpio-generic.c | 2 ++
> > > include/linux/basic_mmio_gpio.h | 1 +
> > > 2 files changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> > > index d2196bf..8c778af 100644
> > > --- a/drivers/gpio/gpio-generic.c
> > > +++ b/drivers/gpio/gpio-generic.c
> > > @@ -531,6 +531,8 @@ static int bgpio_pdev_probe(struct platform_device *pdev)
> > > return err;
> > >
> > > if (pdata) {
> > > + if (pdata->label)
> >
> > Why is this check necessary?
> >
> > > + bgc->gc.label = pdata->label;
>
> Because bgc->gc.label is initialized to a default value in bgpio_init()
> and I don't want to override it with NULL if there is no label passed in
> the platform data.
Ah ha, thanks for clarifying.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 11 Feb 2014, Pawel Moll wrote:
> On Tue, 2014-02-11 at 17:29 +0000, Lee Jones wrote:
> > There must be better
> > ways of passing a name than through pdata, and if there isn't I'd
> > suggest setting up a sysconf pdata struct that others can use in case
> > they wish to expand the functionality further in some other
> > unfathomable way. :)
>
> ... which I proposed in the previous patch :-)
>
> > Alternatively I could define a syscon platform data structure,
> > something like this:
> >
> > struct syscon_platform_data {
> > const char *label;
> > };
So you did.
Yes, this would be the lesser of the two evils.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Define syscon platform data structure that can be used
to define a regmap config name. This is particularly useful
in the regmap debugfs when there is more than one syscon
device registered, to distinguish the register blocks.
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/mfd/syscon.c | 4 ++++
include/linux/platform_data/syscon.h | 8 ++++++++
2 files changed, 12 insertions(+)
create mode 100644 include/linux/platform_data/syscon.h
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 71841f9..854b8d9 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -18,6 +18,7 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/of_platform.h>
+#include <linux/platform_data/syscon.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/mfd/syscon.h>
@@ -126,6 +127,7 @@ static struct regmap_config syscon_regmap_config = {
static int syscon_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
+ struct syscon_platform_data *pdata = dev_get_platdata(dev);
struct syscon *syscon;
struct resource *res;
void __iomem *base;
@@ -143,6 +145,8 @@ static int syscon_probe(struct platform_device *pdev)
return -ENOMEM;
syscon_regmap_config.max_register = res->end - res->start - 3;
+ if (pdata)
+ syscon_regmap_config.name = pdata->label;
syscon->regmap = devm_regmap_init_mmio(dev, base,
&syscon_regmap_config);
if (IS_ERR(syscon->regmap)) {
diff --git a/include/linux/platform_data/syscon.h b/include/linux/platform_data/syscon.h
new file mode 100644
index 0000000..921e59a
--- /dev/null
+++ b/include/linux/platform_data/syscon.h
@@ -0,0 +1,8 @@
+#ifndef PLATFORM_DATA_SYSCON_H
+#define PLATFORM_DATA_SYSCON_H
+
+struct syscon_platform_data {
+ const char *label;
+};
+
+#endif
--
1.8.3.2
This patch adds platform data with label names for syscon
registers in the relevant MFD cells.
Signed-off-by: Pawel Moll <[email protected]>
---
drivers/mfd/vexpress-sysreg.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index ece5ac8..952df84 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -17,6 +17,7 @@
#include <linux/mfd/core.h>
#include <linux/of_address.h>
#include <linux/of_platform.h>
+#include <linux/platform_data/syscon.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/stat.h>
@@ -132,6 +133,10 @@ void __init vexpress_sysreg_early_init(void __iomem *base)
/* The sysreg block is just a random collection of various functions... */
+static struct syscon_platform_data vexpress_sysreg_sys_id_pdata = {
+ .label = "sys_id",
+};
+
static struct bgpio_pdata vexpress_sysreg_sys_led_pdata = {
.label = "sys_led",
.base = -1,
@@ -150,6 +155,14 @@ static struct bgpio_pdata vexpress_sysreg_sys_flash_pdata = {
.ngpio = 1,
};
+static struct syscon_platform_data vexpress_sysreg_sys_misc_pdata = {
+ .label = "sys_misc",
+};
+
+static struct syscon_platform_data vexpress_sysreg_sys_procid_pdata = {
+ .label = "sys_procid",
+};
+
static struct mfd_cell vexpress_sysreg_cells[] = {
{
.name = "syscon",
@@ -157,6 +170,8 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
.resources = (struct resource []) {
DEFINE_RES_MEM(SYS_ID, 0x4),
},
+ .platform_data = &vexpress_sysreg_sys_id_pdata,
+ .pdata_size = sizeof(vexpress_sysreg_sys_id_pdata),
}, {
.name = "basic-mmio-gpio",
.of_compatible = "arm,vexpress-sysreg,sys_led",
@@ -190,12 +205,16 @@ static struct mfd_cell vexpress_sysreg_cells[] = {
.resources = (struct resource []) {
DEFINE_RES_MEM(SYS_MISC, 0x4),
},
+ .platform_data = &vexpress_sysreg_sys_misc_pdata,
+ .pdata_size = sizeof(vexpress_sysreg_sys_misc_pdata),
}, {
.name = "syscon",
.num_resources = 1,
.resources = (struct resource []) {
DEFINE_RES_MEM(SYS_PROCID0, 0x8),
},
+ .platform_data = &vexpress_sysreg_sys_procid_pdata,
+ .pdata_size = sizeof(vexpress_sysreg_sys_procid_pdata),
}, {
.name = "vexpress-syscfg",
.num_resources = 1,
--
1.8.3.2
On Tue, 2014-02-11 at 17:52 +0000, Pawel Moll wrote:
> + const char *label;
Obviously should be a single tab only:
+ const char *label;
Pawel
On Tuesday 11 February 2014 17:10:24 Pawel Moll wrote:
> This series is a set of updates following the infrastructure
> rework and depends on it. It will be finally posted once
> the main series is merged. For the time being I would really
> appreciate feedback and/or (n)acks...
>
I haven't read the patches yet, but on a general level, do
you think this code can be (or should) be shared with
mach-versatile and mach-realview?
One of the things on my (mid-term) to-do list is to completely
convert those two platforms over to being entirely DT based
and having no board specific code so we can actually delete
the directories along with mach-vexpress. The sysreg part
seems to be almost identical, and I wonder if you have an
overview of what the difference is, if any.
If we are going to share that code, we might want to give it
a different name, as vexpress is just the latest platform
in that line.
Arnd
On Tuesday 11 February 2014, Pawel Moll wrote:
> -#define VEXPRESS_HWMON_ATTRS(_name, _label_attr, _input_attr) \
> -struct attribute *vexpress_hwmon_attrs_##_name[] = { \
> - &dev_attr_name.attr, \
> - &dev_attr_##_label_attr.attr, \
> - &sensor_dev_attr_##_input_attr.dev_attr.attr, \
> - NULL \
> -}
> +#define VEXPRESS_HWMON_ATTR_GROUPS(_name, _label_attr, _input_attr) \
> +static struct attribute *vexpress_hwmon_##_name##_attrs[] = { \
> + &dev_attr_##_label_attr.attr, \
> + &sensor_dev_attr_##_input_attr.dev_attr.attr, \
> + NULL \
> +}; \
> +ATTRIBUTE_GROUPS(vexpress_hwmon_##_name)
>
> #if !defined(CONFIG_REGULATOR_VEXPRESS)
> static DEVICE_ATTR(in1_label, S_IRUGO, vexpress_hwmon_label_show, NULL);
> static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, vexpress_hwmon_u32_show,
> NULL, 1000);
> -static VEXPRESS_HWMON_ATTRS(volt, in1_label, in1_input);
> -static struct attribute_group vexpress_hwmon_group_volt = {
> - .attrs = vexpress_hwmon_attrs_volt,
> -};
> +VEXPRESS_HWMON_ATTR_GROUPS(volt, in1_label, in1_input);
Maybe it would be a good time to get rid of the obfuscation here and just
open-code the attribute groups rather than defining another macro.
I understand you want to save a few source lines, but it really helps
readability if you can search for the attribute groups when they
are referenced later. Just because the common driver model code uses
macros with string concatenation doesn't mean it's a good idea to
do the same in a device driver.
Arnd
On Tuesday 11 February 2014, Pawel Moll wrote:
> At this stage of system shutdown procedure the jiffies may
> not be updated anymore, so just use udelay instead.
comment doesn't match code: s/udelay/mdelay/
> + mdelay(1000);
> }
Arnd
On Tue, Feb 11, 2014 at 05:10:33PM +0000, Pawel Moll wrote:
> Use devm_hwmon_device_register_with_groups instead of
> the old-style manual attributes and hwmon device registration.
>
[ ... ]
> static int vexpress_hwmon_probe(struct platform_device *pdev)
> {
> - int err;
> const struct of_device_id *match;
> struct vexpress_hwmon_data *data;
> + const char *name;
> + const struct attribute_group **groups;
>
> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -176,42 +151,19 @@ static int vexpress_hwmon_probe(struct platform_device *pdev)
> return -ENODEV;
>
There is a leftover platform_set_drvdata() above which you can remove;
attributes are now attached to the hwmon device and no longer
to the platform device.
>
> - match = of_match_device(vexpress_hwmon_of_match, &pdev->dev);
> - sysfs_remove_group(&pdev->dev.kobj, match->data);
> + name = of_get_property(pdev->dev.of_node, "compatible", NULL);
Couple of problems, two of which escaped me earlier.
First, can of_get_property ever return NULL ? I think not, just wondering.
Second is a real problem. You have a '-' in the compatible property which is
illegal for the 'name' attribute in hwmon devices. It messes up libsensors
and thus every application using it. Not sure what a good fix (or name)
would be; simplest might be to copy the name string and replace it with '_'.
Sorry for not noticing this earlier; it might actually make sense to submit
a separate patch to address this so we can apply it to the current kernel
and if necessary patch it into earlier kernels.
Third, I noticed that the 'label' attribute is always created but returns
-ENOENT if there is no label. A much better implementation would be to use
.is_visible and not create the label attribute if its devicetree entry
does not exist. I don't know how libsensors reacts to -ENOENT on a read,
but no matter how it does react, it is pretty much undefined.
Again, that should be handled in a separate patch.
I agree with Arnd that it would be nice to get rid of the local macro,
but I won't mandate that.
Thanks,
Guenter
> > Use the device platform data as a regmap config
> > name. This is particularly useful in the regmap
> > debugfs when there is more than one syscon device
> > registered, to distinguish the register blocks.
> >
> > Cc: Samuel Ortiz <[email protected]>
> > Cc: Lee Jones <[email protected]>
> > Signed-off-by: Pawel Moll <[email protected]>
> > ---
> ...
> > syscon_regmap_config.max_register = res->end - res->start - 3;
> > + syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
>
> Is dev_name(&pdev->dev) can be used for such purpose?
Yes of course.
Either use the automatically generated name or over-ride with
dev->init_name prior to registration or call dev_set_name()
manually. Then retrieve with Alexander's suggestion.
Is there any technical reason why this is not possible with your
implementation?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 2014-02-12 at 08:26 +0000, Lee Jones wrote:
> > > Use the device platform data as a regmap config
> > > name. This is particularly useful in the regmap
> > > debugfs when there is more than one syscon device
> > > registered, to distinguish the register blocks.
> > >
> > > Cc: Samuel Ortiz <[email protected]>
> > > Cc: Lee Jones <[email protected]>
> > > Signed-off-by: Pawel Moll <[email protected]>
> > > ---
> > ...
> > > syscon_regmap_config.max_register = res->end - res->start - 3;
> > > + syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
> >
> > Is dev_name(&pdev->dev) can be used for such purpose?
>
> Yes of course.
>
> Either use the automatically generated name or over-ride with
> dev->init_name prior to registration or call dev_set_name()
> manually. Then retrieve with Alexander's suggestion.
>
> Is there any technical reason why this is not possible with your
> implementation?
Hold on, guys. Let me just point out that we're talking "non-DT"
platform devices here (either statically defined struct
platform_device-s or - my case - the MFD cells).
In this case device/driver matching relies completely on device name.
Either the pdev->name must be identical (strcmp) to pdrv->name, or the
pdev->name must be identical (strcmp again) to one of the pdev->id_table
entries. See platform_match() in driver/base/platform.c for more
details.
Therefore the dev_name(&pdev->dev) on a non-DT-originating sysconf
devince will always return "sysconf.*", unless you're ready to maintain
a growing syscon_ids[] list. If so, I will have to add three entries
there ("sys_id", "sys_misc" and "sys_procid"). I hope you are not
seriously considering this idea :-) After all that's what the
platform_data was invented for.
Pawel
> > > > Use the device platform data as a regmap config
> > > > name. This is particularly useful in the regmap
> > > > debugfs when there is more than one syscon device
> > > > registered, to distinguish the register blocks.
> > > >
> > > > Cc: Samuel Ortiz <[email protected]>
> > > > Cc: Lee Jones <[email protected]>
> > > > Signed-off-by: Pawel Moll <[email protected]>
> > > > ---
> > > ...
> > > > syscon_regmap_config.max_register = res->end - res->start - 3;
> > > > + syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
> > >
> > > Is dev_name(&pdev->dev) can be used for such purpose?
> >
> > Yes of course.
> >
> > Either use the automatically generated name or over-ride with
> > dev->init_name prior to registration or call dev_set_name()
> > manually. Then retrieve with Alexander's suggestion.
> >
> > Is there any technical reason why this is not possible with your
> > implementation?
>
> Hold on, guys. Let me just point out that we're talking "non-DT"
> platform devices here (either statically defined struct
> platform_device-s or - my case - the MFD cells).
>
> In this case device/driver matching relies completely on device name.
> Either the pdev->name must be identical (strcmp) to pdrv->name, or the
> pdev->name must be identical (strcmp again) to one of the pdev->id_table
> entries. See platform_match() in driver/base/platform.c for more
> details.
>
> Therefore the dev_name(&pdev->dev) on a non-DT-originating sysconf
> devince will always return "sysconf.*", unless you're ready to maintain
> a growing syscon_ids[] list. If so, I will have to add three entries
> there ("sys_id", "sys_misc" and "sys_procid"). I hope you are not
> seriously considering this idea :-) After all that's what the
> platform_data was invented for.
Ah, I see your predicament. I guess that is a limitation of the was
syscon works. Usually we'd use match tables to differentiate between
various supported devices, but I guess the 'supported devices' list
for syscon is pretty limitless. In which case I support your second
implementation (adding a platform_data container) for the use of
arbitrary/useful-common names.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> Define syscon platform data structure that can be used
> to define a regmap config name. This is particularly useful
> in the regmap debugfs when there is more than one syscon
> device registered, to distinguish the register blocks.
>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> drivers/mfd/syscon.c | 4 ++++
> include/linux/platform_data/syscon.h | 8 ++++++++
> 2 files changed, 12 insertions(+)
> create mode 100644 include/linux/platform_data/syscon.h
<snip>
> +#ifndef PLATFORM_DATA_SYSCON_H
> +#define PLATFORM_DATA_SYSCON_H
> +
> +struct syscon_platform_data {
> + const char *label;
> +};
Provided you fix this up:
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> This patch adds platform data with label names for syscon
> registers in the relevant MFD cells.
>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> drivers/mfd/vexpress-sysreg.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
Acked-by: Lee Jones <[email protected]>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 2014-02-12 at 11:27 +0000, Alexander Shiyan wrote:
> Yeah, I gave up the idea to use the syscon_ids[] to separate devices for
> non-DT case.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/mfd/syscon.c?id=5104d2656d4874c51868dc7182016e9501ec99ca
> Instead, I use a hard definition for pdev->id, so that the names of
> syscon-devices are different and can be obtained from the driver
> it uses with syscon_regmap_lookup_by_pdevname().
It is a sort-of-solution (I personally dislike magic numbers, but this
is a separate discussion) for a completely-non-DT case, where you can
guarantee limited number of syscon devices.
In my case I have a DT system, where some of the MFD devices *may*
register syscon cells... No way to enforce ordering.
> I understand this topic correct?
The main idea here is to attach a meaningful label to the syscon
regmaps. My example:
/ # ls -d1 /sys/kernel/debug/regmap/syscon.*
/sys/kernel/debug/regmap/syscon.0.auto
/sys/kernel/debug/regmap/syscon.4.auto
/sys/kernel/debug/regmap/syscon.5.auto
vs
/ # ls -d /sys/kernel/debug/regmap/syscon.*
/sys/kernel/debug/regmap/syscon.0.auto-sys_id
/sys/kernel/debug/regmap/syscon.4.auto-sys_misc
/sys/kernel/debug/regmap/syscon.5.auto-sys_procid
Of course one could also define syscon_regmap_lookup_by_label() (I don't
really need it right now so didn't go that way)
Pawel
On Wed, 2014-02-12 at 02:49 +0000, Guenter Roeck wrote:
> On Tue, Feb 11, 2014 at 05:10:33PM +0000, Pawel Moll wrote:
> > Use devm_hwmon_device_register_with_groups instead of
> > the old-style manual attributes and hwmon device registration.
> >
>
> [ ... ]
>
> > static int vexpress_hwmon_probe(struct platform_device *pdev)
> > {
> > - int err;
> > const struct of_device_id *match;
> > struct vexpress_hwmon_data *data;
> > + const char *name;
> > + const struct attribute_group **groups;
> >
> > data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> > @@ -176,42 +151,19 @@ static int vexpress_hwmon_probe(struct platform_device *pdev)
> > return -ENODEV;
> >
>
> There is a leftover platform_set_drvdata() above which you can remove;
> attributes are now attached to the hwmon device and no longer
> to the platform device.
Right, missed this fact. Will remove.
BTW. It's cool that the attributes live in the class device now. You
probably don't remember, but in the very first version of the driver I
was trying to get this point with some tricks that weren't taken
nicely ;-)
> > - match = of_match_device(vexpress_hwmon_of_match, &pdev->dev);
> > - sysfs_remove_group(&pdev->dev.kobj, match->data);
> > + name = of_get_property(pdev->dev.of_node, "compatible", NULL);
>
> Couple of problems, two of which escaped me earlier.
> First, can of_get_property ever return NULL ? I think not, just wondering.
Generally it can, but not in this particular case, no. The device
wouldn't get bound to the driver if it lacked the "compatible" property.
> Second is a real problem. You have a '-' in the compatible property which is
> illegal for the 'name' attribute in hwmon devices. It messes up libsensors
> and thus every application using it. Not sure what a good fix (or name)
> would be; simplest might be to copy the name string and replace it with '_'.
> Sorry for not noticing this earlier; it might actually make sense to submit
> a separate patch to address this so we can apply it to the current kernel
> and if necessary patch it into earlier kernels.
Ok, will do. Either with s/-/_/ or with a name lookup table.
Interestingly enough I never observed any problem with libsensors (3.3.2
was the one I used) on my board...
> Third, I noticed that the 'label' attribute is always created but returns
> -ENOENT if there is no label. A much better implementation would be to use
> .is_visible and not create the label attribute if its devicetree entry
> does not exist. I don't know how libsensors reacts to -ENOENT on a read,
> but no matter how it does react, it is pretty much undefined.
> Again, that should be handled in a separate patch.
> I agree with Arnd that it would be nice to get rid of the local macro,
> but I won't mandate that.
I actually prefer to have the structures unfolded, but was just trying
to mimic the trend in other hwmon drivers. No problem - will change this
gladly.
Pawel
On Tue, 2014-02-11 at 20:59 +0000, Arnd Bergmann wrote:
> On Tuesday 11 February 2014, Pawel Moll wrote:
> > At this stage of system shutdown procedure the jiffies may
> > not be updated anymore, so just use udelay instead.
>
> comment doesn't match code: s/udelay/mdelay/
>
> > + mdelay(1000);
> > }
Uh, right. Well spotted. Will fix.
Pawel
On Wed, 2014-02-12 at 11:56 +0000, Pawel Moll wrote:
> > Third, I noticed that the 'label' attribute is always created but returns
> > -ENOENT if there is no label. A much better implementation would be to use
> > .is_visible and not create the label attribute if its devicetree entry
> > does not exist. I don't know how libsensors reacts to -ENOENT on a read,
> > but no matter how it does react, it is pretty much undefined.
> > Again, that should be handled in a separate patch.
Missed that one - will do as well.
Pawel
On Tue, 2014-02-11 at 19:28 +0000, Arnd Bergmann wrote:
> On Tuesday 11 February 2014 17:10:24 Pawel Moll wrote:
> > This series is a set of updates following the infrastructure
> > rework and depends on it. It will be finally posted once
> > the main series is merged. For the time being I would really
> > appreciate feedback and/or (n)acks...
>
> I haven't read the patches yet, but on a general level, do
> you think this code can be (or should) be shared with
> mach-versatile and mach-realview?
Haven't worked with the old ones too much, I just had a look at a couple
of TRMs... The sysregs look similar, don't they? Deceitfully
similar... ;-)
Let me start from the VE side and classify the registers into functional
groups, looking at them from a MFD cells point of view:
- general ID registers like SYS_ID, SYS_MISC, SYS_PROCIDx - either not
really useful for drivers or different bitmasks if needed (eg. the
MASTERSITE bit in SYS_MISC); in other words - good candidates for syscon
if at all interesting
- de-facto GPIO registers like SYS_LED, SYS_MCI, SYS_FLASH - easily
covered via "basic-mmio-gpio" driver; can be simply exposed in the tree
as subnodes
- flags (SYS_*FLAGS*) - used pretty much only for SMP booting; that's
where, I believe, we could unify and move the DT-based operations into
plat_versatile; it is too early for the device model though, so out of
the sysreg driver scope, really (even if the code lives here for now)
- counters (SYS_100HZ, SYS_24MHZ) - the 24MHz one is used on some of the
platforms as a sched clock source; in the fourth patch of the second
series I moved it out into drivers/clocksource - it could be easily
generalised and used across more platforms, but again - it's early code,
so out of sysreg driver scope
- system configuration (SYS_CFG*) - that's the main difference between
VE and its predecessors; the older seem to provide separate system
registers to control infrastructure like clocks etc. (eg. SYS_OSC* on
the realview boards), while here we have the (whatever you call it ;-)
config bus
- other registers that are currently not used at all, but most of them
differ either in naming (and location) or in offset.
All this leads, I believe, to conclusion that they definitely deserve
different compatible values. Therefore they would also require different
mfdcells definitions. Of course we could have more than one in a single
file and pick one via matching table but:
1. There is always that little "extra" (like the master site
configuration on VE, which doesn't happen on the previous ones)
2. I'd rather not to have the non-VE stuff in arm64 kernel.
So, to summarize, I think we need separate files for at least main
families of the sysreg drivers (I didn't spent enough time on this to be
able to find commonalities and differences inside realview and versatile
families), but try to share as much as possible of the "functional"
drivers (with the sched clock and the smp operations being the most
obvious candidates).
> One of the things on my (mid-term) to-do list is to completely
> convert those two platforms over to being entirely DT based
> and having no board specific code so we can actually delete
> the directories along with mach-vexpress
And I sympathise with this goal :-) (interestingly Olof in the past
expressed strong and opposite opinion in the subject ;-)
If you have a look at the last patch of the second series, you'll find
that what's left in the VEXPRESS_DT machine are smp operations and
l2x0_of_init. Then, assuming that we finally get some answer to the CLCD
problem (latest in this subject happened here:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/268037 - I really
lost all hopes for CDF getting anywhere...) removal of the non-DT code
is a matter of a single patch (and a massive negative stat :-). From
there there's only a bunch of PM-related files in arch/arm/mach-vexpress
that I guess could be moved somewhere else (that's what Olof didn't
really like).
Pawel
On Wed, Feb 12, 2014 at 11:56:36AM +0000, Pawel Moll wrote:
>
> > I agree with Arnd that it would be nice to get rid of the local macro,
> > but I won't mandate that.
>
> I actually prefer to have the structures unfolded, but was just trying
> to mimic the trend in other hwmon drivers. No problem - will change this
> gladly.
>
In general we tend to discourage people to use their own macros -
most of the time it just makes the code more difficult to read.
Guenter
On Tue, Feb 11, 2014 at 6:10 PM, Pawel Moll <[email protected]> wrote:
> ... so debugfs interfaces are easier to use.
>
> Cc: Linus Walleij <[email protected]>
> Cc: Alexandre Courbot <[email protected]>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
This doesn't apply to the v3.14-rc1-based GPIO tree. Can you rebase it
with Lee's ACK so I can apply this?
Yours,
Linus Walleij
On Thu, 2014-02-13 at 13:08 +0000, Linus Walleij wrote:
> On Tue, Feb 11, 2014 at 6:10 PM, Pawel Moll <[email protected]> wrote:
>
> > ... so debugfs interfaces are easier to use.
> >
> > Cc: Linus Walleij <[email protected]>
> > Cc: Alexandre Courbot <[email protected]>
> > Cc: Samuel Ortiz <[email protected]>
> > Cc: Lee Jones <[email protected]>
> > Signed-off-by: Pawel Moll <[email protected]>
>
> This doesn't apply to the v3.14-rc1-based GPIO tree. Can you rebase it
> with Lee's ACK so I can apply this?
Uh, it won't apply. This series depends on the one reworking sysregs.
I'll send them separately once it's ready. Sorry about confusion and
thanks for trying!
Pawel
On Tue, Feb 11, 2014 at 05:10:25PM +0000, Pawel Moll wrote:
> It is normally preferable to yield the task
> waiting for syscfg operations (that can take
> up to dozens of milliseconds), but when the
> system is shutting down it may not be possible.
Really? What happens at shutdown that prevents things from being
scheduled? Why would you want to busy-loop instead at this point in
time?
thanks,
greg k-h
Hello.
Вторник, 11 февраля 2014, 17:10 UTC от Pawel Moll <[email protected]>:
> Use the device platform data as a regmap config
> name. This is particularly useful in the regmap
> debugfs when there is more than one syscon device
> registered, to distinguish the register blocks.
>
> Cc: Samuel Ortiz <[email protected]>
> Cc: Lee Jones <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
...
> syscon_regmap_config.max_register = res->end - res->start - 3;
> + syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
Is dev_name(&pdev->dev) can be used for such purpose?
Thanks.
---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Среда, 12 февраля 2014, 11:06 UTC от Pawel Moll <[email protected]>:
> On Wed, 2014-02-12 at 08:26 +0000, Lee Jones wrote:
> > > > Use the device platform data as a regmap config
> > > > name. This is particularly useful in the regmap
> > > > debugfs when there is more than one syscon device
> > > > registered, to distinguish the register blocks.
> > > >
> > > > Cc: Samuel Ortiz <[email protected]>
> > > > Cc: Lee Jones <[email protected]>
> > > > Signed-off-by: Pawel Moll <[email protected]>
> > > > ---
> > > ...
> > > > syscon_regmap_config.max_register = res->end - res->start - 3;
> > > > + syscon_regmap_config.name = dev_get_platdata(&pdev->dev);
> > >
> > > Is dev_name(&pdev->dev) can be used for such purpose?
> >
> > Yes of course.
> >
> > Either use the automatically generated name or over-ride with
> > dev->init_name prior to registration or call dev_set_name()
> > manually. Then retrieve with Alexander's suggestion.
> >
> > Is there any technical reason why this is not possible with your
> > implementation?
>
> Hold on, guys. Let me just point out that we're talking "non-DT"
> platform devices here (either statically defined struct
> platform_device-s or - my case - the MFD cells).
>
> In this case device/driver matching relies completely on device name.
> Either the pdev->name must be identical (strcmp) to pdrv->name, or the
> pdev->name must be identical (strcmp again) to one of the pdev->id_table
> entries. See platform_match() in driver/base/platform.c for more
> details.
>
> Therefore the dev_name(&pdev->dev) on a non-DT-originating sysconf
> devince will always return "sysconf.*", unless you're ready to maintain
> a growing syscon_ids[] list. If so, I will have to add three entries
> there ("sys_id", "sys_misc" and "sys_procid"). I hope you are not
> seriously considering this idea :-) After all that's what the
> platform_data was invented for.
Yeah, I gave up the idea to use the syscon_ids[] to separate devices for
non-DT case.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/mfd/syscon.c?id=5104d2656d4874c51868dc7182016e9501ec99ca
Instead, I use a hard definition for pdev->id, so that the names of
syscon-devices are different and can be obtained from the driver
it uses with syscon_regmap_lookup_by_pdevname().
I understand this topic correct?
---
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Adding Linus W...
On Tue, Feb 11, 2014 at 11:10 AM, Pawel Moll <[email protected]> wrote:
> This patch adds a trival sched clock source using free
> running, 24MHz clocked counter present in the ARM Ltd.
> Versatile Express platform's System Registers block.
>
> This code replaces the call in the VE machine code.
>
> Cc: Daniel Lezcano <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Signed-off-by: Pawel Moll <[email protected]>
> ---
> arch/arm/mach-vexpress/v2m.c | 2 --
> drivers/clocksource/Kconfig | 9 +++++++++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/vexpress.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 50 insertions(+), 2 deletions(-)
> create mode 100644 drivers/clocksource/vexpress.c
>
> diff --git a/arch/arm/mach-vexpress/v2m.c b/arch/arm/mach-vexpress/v2m.c
> index 90f04c9..d8a9fd7 100644
> --- a/arch/arm/mach-vexpress/v2m.c
> +++ b/arch/arm/mach-vexpress/v2m.c
> @@ -418,8 +418,6 @@ void __init v2m_dt_init_early(void)
> pr_warning("vexpress: DT HBI (%x) is not matching "
> "hardware (%x)!\n", dt_hbi, hbi);
> }
> -
> - versatile_sched_clock_init(vexpress_get_24mhz_clock_base(), 24000000);
> }
>
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index cd6950f..9799744 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -140,3 +140,12 @@ config VF_PIT_TIMER
> bool
> help
> Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
> +
> +config CLKSRC_VEXPRESS
> + bool
> + depends on MFD_VEXPRESS_SYSREG
But you don't really depend on this...
> + depends on GENERIC_SCHED_CLOCK
I think this should be a select, not a depends.
> + select CLKSRC_OF
> + default y
> + help
> + Simple provider of sched clock on ARM Versatile Express platform.
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index c7ca50a..1051a23 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
> obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o
> obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o
> obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST) += dummy_timer.o
> +obj-$(CONFIG_CLKSRC_VEXPRESS) += vexpress.o
> diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
> new file mode 100644
> index 0000000..55b8ab4
> --- /dev/null
> +++ b/drivers/clocksource/vexpress.c
> @@ -0,0 +1,40 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * Copyright (C) 2013 ARM Limited
Did you write this last year?
> + */
> +
> +#include <linux/clocksource.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/sched_clock.h>
> +
> +#define SYS_24MHZ 0x05c
> +
> +static void __iomem *vexpress_sys_24mhz;
> +
> +static u32 notrace vexpress_sys_24mhz_read(void)
> +{
> + return readl(vexpress_sys_24mhz);
> +}
> +
> +static void __init vexpress_sched_clock_init(struct device_node *node)
> +{
> + void __iomem *base = of_iomap(node, 0);
> +
> + if (!base)
> + return;
> +
> + vexpress_sys_24mhz = base + SYS_24MHZ;
> +
> + setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
This frequency should come from a DT clock binding. You will have to
fallback to 24MHz for backwards compatibility though.
> +}
Wouldn't this code work for Versatile and Realview ARM reference
boards? Even the register offset is the same.
> +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> + vexpress_sched_clock_init);
> --
> 1.8.3.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Rob,
Thanks for reminding me about this stuff. I'll get around and re-spin
the series this week.
On Wed, 2014-04-16 at 14:56 +0100, Rob Herring wrote:
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index cd6950f..9799744 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -140,3 +140,12 @@ config VF_PIT_TIMER
> > bool
> > help
> > Support for Period Interrupt Timer on Freescale Vybrid Family SoCs.
> > +
> > +config CLKSRC_VEXPRESS
> > + bool
> > + depends on MFD_VEXPRESS_SYSREG
>
> But you don't really depend on this...
Hm. Strictly speaking it's true (no code level dependency) but if one
doesn't build sysreg driver, one doesn't care
> > + depends on GENERIC_SCHED_CLOCK
>
> I think this should be a select, not a depends.
Don't think so, no. It's being selected by an arch.
> > + select CLKSRC_OF
> > + default y
> > + help
> > + Simple provider of sched clock on ARM Versatile Express platform.
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index c7ca50a..1051a23 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -37,3 +37,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER) += arm_arch_timer.o
> > obj-$(CONFIG_ARM_GLOBAL_TIMER) += arm_global_timer.o
> > obj-$(CONFIG_CLKSRC_METAG_GENERIC) += metag_generic.o
> > obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST) += dummy_timer.o
> > +obj-$(CONFIG_CLKSRC_VEXPRESS) += vexpress.o
> > diff --git a/drivers/clocksource/vexpress.c b/drivers/clocksource/vexpress.c
> > new file mode 100644
> > index 0000000..55b8ab4
> > --- /dev/null
> > +++ b/drivers/clocksource/vexpress.c
> > @@ -0,0 +1,40 @@
> > +/*
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that 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.
> > + *
> > + * Copyright (C) 2013 ARM Limited
>
> Did you write this last year?
Yes. Can bump it up anyway.
> > + */
> > +
> > +#include <linux/clocksource.h>
> > +#include <linux/io.h>
> > +#include <linux/of_address.h>
> > +#include <linux/sched_clock.h>
> > +
> > +#define SYS_24MHZ 0x05c
> > +
> > +static void __iomem *vexpress_sys_24mhz;
> > +
> > +static u32 notrace vexpress_sys_24mhz_read(void)
> > +{
> > + return readl(vexpress_sys_24mhz);
> > +}
> > +
> > +static void __init vexpress_sched_clock_init(struct device_node *node)
> > +{
> > + void __iomem *base = of_iomap(node, 0);
> > +
> > + if (!base)
> > + return;
> > +
> > + vexpress_sys_24mhz = base + SYS_24MHZ;
> > +
> > + setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
>
> This frequency should come from a DT clock binding. You will have to
> fallback to 24MHz for backwards compatibility though.
I don't see why would it go to the binding. You may have noticed the
register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
*knows* what the frequency is.
> > +}
>
> Wouldn't this code work for Versatile and Realview ARM reference
> boards? Even the register offset is the same.
>
> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> > + vexpress_sched_clock_init);
I guess it would, yes. The sysregs are annoyingly similar and different
at the same time.
One could of course try to come up with a "generic mmio clock source"
binding, taking the frequency as a property, but don't count on me doing
this... ;-)
Pawel
On Wed, Apr 16, 2014 at 9:22 AM, Pawel Moll <[email protected]> wrote:
> Hi Rob,
>
> Thanks for reminding me about this stuff. I'll get around and re-spin
> the series this week.
>
[snip]
>> > + setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
>>
>> This frequency should come from a DT clock binding. You will have to
>> fallback to 24MHz for backwards compatibility though.
>
> I don't see why would it go to the binding. You may have noticed the
> register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
> *knows* what the frequency is.
A 24MHz clock is fed to this h/w block to be used by the counter in
the block. The DT should describe that.
>
>> > +}
>>
>> Wouldn't this code work for Versatile and Realview ARM reference
>> boards? Even the register offset is the same.
>>
>> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
>> > + vexpress_sched_clock_init);
>
> I guess it would, yes. The sysregs are annoyingly similar and different
> at the same time.
>
> One could of course try to come up with a "generic mmio clock source"
> binding, taking the frequency as a property, but don't count on me doing
> this... ;-)
I'm not asking for that. Just take care of all ARM Ltd boards which
have the exact same 24MHz counter at offset 0x5C.
Rob
On Wed, 2014-04-16 at 15:45 +0100, Rob Herring wrote:
> >> > + setup_sched_clock(vexpress_sys_24mhz_read, 32, 24000000);
> >>
> >> This frequency should come from a DT clock binding. You will have to
> >> fallback to 24MHz for backwards compatibility though.
> >
> > I don't see why would it go to the binding. You may have noticed the
> > register is called "SYS_24MHZ", not "SYS_RANDOMCLOCK". The driver
> > *knows* what the frequency is.
>
> A 24MHz clock is fed to this h/w block to be used by the counter in
> the block. The DT should describe that.
I even have a clk24mhz in the motherboard's dtsi, so adding a phandle to
the node is trivial, can do it. But my point is that, whatever this
clock is (24MHz, 12MHz then multiplied by 2 inside sysreg, 48MHz divided
by 2 etc.), the value in this register is incremented every 1/24 uS by
definition. And the driver can rely on this. Otherwise we're really
talking about a generic mmio-counter-based clock source, with a
clock-frequency property or a clock phandle (and a multiplier/divider
then?).
> >> > +}
> >>
> >> Wouldn't this code work for Versatile and Realview ARM reference
> >> boards? Even the register offset is the same.
> >>
> >> > +CLOCKSOURCE_OF_DECLARE(vexpress, "arm,vexpress-sysreg",
> >> > + vexpress_sched_clock_init);
> >
> > I guess it would, yes. The sysregs are annoyingly similar and different
> > at the same time.
> >
> > One could of course try to come up with a "generic mmio clock source"
> > binding, taking the frequency as a property, but don't count on me doing
> > this... ;-)
>
> I'm not asking for that. Just take care of all ARM Ltd boards which
> have the exact same 24MHz counter at offset 0x5C.
Yes, this is doable by all means. I'll rename it to
"clocksource/versatile.c" so we can add relevant compatible values.
Paweł