| Message ID | 1386166641-7567-3-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 04, 2013 at 02:17:19PM +0000, Andrew Lunn wrote: > Add a platform driver definition to instantiate the dove cpufreq > driver. Also indicate the ARCH has cpufreq support, so allowing the > cpufreq framework to be enabled. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > --- > Make resource names consistent. > Fix length of ranges > --- > arch/arm/Kconfig | 1 + > arch/arm/mach-dove/board-dt.c | 3 +++ > arch/arm/mach-dove/common.c | 36 ++++++++++++++++++++++++++++++++++ > arch/arm/mach-dove/common.h | 1 + > arch/arm/mach-dove/include/mach/dove.h | 1 + > 5 files changed, 42 insertions(+) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index c1f1a7eee953..908b02f8d901 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -500,6 +500,7 @@ config ARCH_IXP4XX > > config ARCH_DOVE > bool "Marvell Dove" > + select ARCH_HAS_CPUFREQ > select ARCH_REQUIRE_GPIOLIB > select CPU_PJ4 > select GENERIC_CLOCKEVENTS > diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c > index 49fa9abd09da..845e8b328432 100644 > --- a/arch/arm/mach-dove/board-dt.c > +++ b/arch/arm/mach-dove/board-dt.c > @@ -27,6 +27,9 @@ static void __init dove_dt_init(void) > tauros2_init(0); > #endif > BUG_ON(mvebu_mbus_dt_init()); > + > + dove_cpufreq_init(); > + > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > } > > diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c > index c122bcff9f7c..323fcd4b27b6 100644 > --- a/arch/arm/mach-dove/common.c > +++ b/arch/arm/mach-dove/common.c > @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void) > platform_device_register(&dove_sdio1); > } > > +/***************************************************************************** > + * CPU Frequency > + ****************************************************************************/ > +static struct resource dove_cpufreq_resources[] = { > + [0] = { > + .start = DOVE_PMU_PHYS_BASE, > + .end = DOVE_PMU_PHYS_BASE + 0x7, > + .flags = IORESOURCE_MEM, > + .name = "cpufreq: DFS" > + }, > + [1] = { > + .start = DOVE_PMU_PHYS_BASE + 0x8000, > + .end = DOVE_PMU_PHYS_BASE + 0x8003, > + .flags = IORESOURCE_MEM, > + .name = "cpufreq: PMU CR" > + }, > + [2] = { > + .start = DOVE_PMU_PHYS_BASE + 0x0044, > + .end = DOVE_PMU_PHYS_BASE + 0x0047, > + .flags = IORESOURCE_MEM, > + .name = "cpufreq: PMU Clk Div" > + }, > +}; > + > +static struct platform_device dove_cpufreq_device = { > + .name = "dove-cpufreq", > + .id = -1, > + .num_resources = ARRAY_SIZE(dove_cpufreq_resources), > + .resource = dove_cpufreq_resources, > +}; > + > +void __init dove_cpufreq_init(void) > +{ > + platform_device_register(&dove_cpufreq_device); > +} Please describe your power management unit in the DT. Your hardcoding physical addresses that can easily be described in the DT, and you don't appear to have an early probing requirement. > + > void __init dove_setup_cpu_wins(void) > { > /* > diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h > index 1d725224d146..5c9a77bdd442 100644 > --- a/arch/arm/mach-dove/common.h > +++ b/arch/arm/mach-dove/common.h > @@ -44,6 +44,7 @@ void dove_spi1_init(void); > void dove_i2c_init(void); > void dove_sdio0_init(void); > void dove_sdio1_init(void); > +void dove_cpufreq_init(void); > void dove_restart(enum reboot_mode, const char *); > > #endif > diff --git a/arch/arm/mach-dove/include/mach/dove.h b/arch/arm/mach-dove/include/mach/dove.h > index 0c4b35f4ee5b..48db186ae6bc 100644 > --- a/arch/arm/mach-dove/include/mach/dove.h > +++ b/arch/arm/mach-dove/include/mach/dove.h > @@ -144,6 +144,7 @@ > #define DOVE_SD0_GPIO_SEL (1 << 0) > > /* Power Management */ > +#define DOVE_PMU_PHYS_BASE (DOVE_SB_REGS_PHYS_BASE + 0xd0000) > #define DOVE_PMU_VIRT_BASE (DOVE_SB_REGS_VIRT_BASE + 0xd0000) > #define DOVE_PMU_SIG_CTRL (DOVE_PMU_VIRT_BASE + 0x802c) If you need to describe the large block this appears to be in, do that too. Thanks, Mark.
On Wed, Dec 04, 2013 at 02:36:28PM +0000, Mark Rutland wrote: > On Wed, Dec 04, 2013 at 02:17:19PM +0000, Andrew Lunn wrote: > > Add a platform driver definition to instantiate the dove cpufreq > > driver. Also indicate the ARCH has cpufreq support, so allowing the > > cpufreq framework to be enabled. > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > --- > > Make resource names consistent. > > Fix length of ranges > > --- > > arch/arm/Kconfig | 1 + > > arch/arm/mach-dove/board-dt.c | 3 +++ > > arch/arm/mach-dove/common.c | 36 ++++++++++++++++++++++++++++++++++ > > arch/arm/mach-dove/common.h | 1 + > > arch/arm/mach-dove/include/mach/dove.h | 1 + > > 5 files changed, 42 insertions(+) > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index c1f1a7eee953..908b02f8d901 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -500,6 +500,7 @@ config ARCH_IXP4XX > > > > config ARCH_DOVE > > bool "Marvell Dove" > > + select ARCH_HAS_CPUFREQ > > select ARCH_REQUIRE_GPIOLIB > > select CPU_PJ4 > > select GENERIC_CLOCKEVENTS > > diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c > > index 49fa9abd09da..845e8b328432 100644 > > --- a/arch/arm/mach-dove/board-dt.c > > +++ b/arch/arm/mach-dove/board-dt.c > > @@ -27,6 +27,9 @@ static void __init dove_dt_init(void) > > tauros2_init(0); > > #endif > > BUG_ON(mvebu_mbus_dt_init()); > > + > > + dove_cpufreq_init(); > > + > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > } > > > > diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c > > index c122bcff9f7c..323fcd4b27b6 100644 > > --- a/arch/arm/mach-dove/common.c > > +++ b/arch/arm/mach-dove/common.c > > @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void) > > platform_device_register(&dove_sdio1); > > } > > > > +/***************************************************************************** > > + * CPU Frequency > > + ****************************************************************************/ > > +static struct resource dove_cpufreq_resources[] = { > > + [0] = { > > + .start = DOVE_PMU_PHYS_BASE, > > + .end = DOVE_PMU_PHYS_BASE + 0x7, > > + .flags = IORESOURCE_MEM, > > + .name = "cpufreq: DFS" > > + }, > > + [1] = { > > + .start = DOVE_PMU_PHYS_BASE + 0x8000, > > + .end = DOVE_PMU_PHYS_BASE + 0x8003, > > + .flags = IORESOURCE_MEM, > > + .name = "cpufreq: PMU CR" > > + }, > > + [2] = { > > + .start = DOVE_PMU_PHYS_BASE + 0x0044, > > + .end = DOVE_PMU_PHYS_BASE + 0x0047, > > + .flags = IORESOURCE_MEM, > > + .name = "cpufreq: PMU Clk Div" > > + }, > > +}; > > + > > +static struct platform_device dove_cpufreq_device = { > > + .name = "dove-cpufreq", > > + .id = -1, > > + .num_resources = ARRAY_SIZE(dove_cpufreq_resources), > > + .resource = dove_cpufreq_resources, > > +}; > > + > > +void __init dove_cpufreq_init(void) > > +{ > > + platform_device_register(&dove_cpufreq_device); > > +} > > Please describe your power management unit in the DT. Your hardcoding > physical addresses that can easily be described in the DT, and you don't > appear to have an early probing requirement. Hi Mark We have been here before, with the kirkwood cpufreq driver. My v1 of that driver had the cpufreq driver fully described in DT. My binding got shot down and i was told that pseudo devices, like cpufreq, cpuidle, should not be in DT, and a platform driver was the way to go. So that is how kirkwood cpufreq works and dove follows that. Do you want to reverse that decision now? Thanks Andrew
On Wed, Dec 04, 2013 at 03:02:28PM +0000, Andrew Lunn wrote: > On Wed, Dec 04, 2013 at 02:36:28PM +0000, Mark Rutland wrote: > > On Wed, Dec 04, 2013 at 02:17:19PM +0000, Andrew Lunn wrote: > > > Add a platform driver definition to instantiate the dove cpufreq > > > driver. Also indicate the ARCH has cpufreq support, so allowing the > > > cpufreq framework to be enabled. > > > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > > > --- > > > Make resource names consistent. > > > Fix length of ranges > > > --- > > > arch/arm/Kconfig | 1 + > > > arch/arm/mach-dove/board-dt.c | 3 +++ > > > arch/arm/mach-dove/common.c | 36 ++++++++++++++++++++++++++++++++++ > > > arch/arm/mach-dove/common.h | 1 + > > > arch/arm/mach-dove/include/mach/dove.h | 1 + > > > 5 files changed, 42 insertions(+) > > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > > index c1f1a7eee953..908b02f8d901 100644 > > > --- a/arch/arm/Kconfig > > > +++ b/arch/arm/Kconfig > > > @@ -500,6 +500,7 @@ config ARCH_IXP4XX > > > > > > config ARCH_DOVE > > > bool "Marvell Dove" > > > + select ARCH_HAS_CPUFREQ > > > select ARCH_REQUIRE_GPIOLIB > > > select CPU_PJ4 > > > select GENERIC_CLOCKEVENTS > > > diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c > > > index 49fa9abd09da..845e8b328432 100644 > > > --- a/arch/arm/mach-dove/board-dt.c > > > +++ b/arch/arm/mach-dove/board-dt.c > > > @@ -27,6 +27,9 @@ static void __init dove_dt_init(void) > > > tauros2_init(0); > > > #endif > > > BUG_ON(mvebu_mbus_dt_init()); > > > + > > > + dove_cpufreq_init(); > > > + > > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > > } > > > > > > diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c > > > index c122bcff9f7c..323fcd4b27b6 100644 > > > --- a/arch/arm/mach-dove/common.c > > > +++ b/arch/arm/mach-dove/common.c > > > @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void) > > > platform_device_register(&dove_sdio1); > > > } > > > > > > +/***************************************************************************** > > > + * CPU Frequency > > > + ****************************************************************************/ > > > +static struct resource dove_cpufreq_resources[] = { > > > + [0] = { > > > + .start = DOVE_PMU_PHYS_BASE, > > > + .end = DOVE_PMU_PHYS_BASE + 0x7, > > > + .flags = IORESOURCE_MEM, > > > + .name = "cpufreq: DFS" > > > + }, > > > + [1] = { > > > + .start = DOVE_PMU_PHYS_BASE + 0x8000, > > > + .end = DOVE_PMU_PHYS_BASE + 0x8003, > > > + .flags = IORESOURCE_MEM, > > > + .name = "cpufreq: PMU CR" > > > + }, > > > + [2] = { > > > + .start = DOVE_PMU_PHYS_BASE + 0x0044, > > > + .end = DOVE_PMU_PHYS_BASE + 0x0047, > > > + .flags = IORESOURCE_MEM, > > > + .name = "cpufreq: PMU Clk Div" > > > + }, > > > +}; > > > + > > > +static struct platform_device dove_cpufreq_device = { > > > + .name = "dove-cpufreq", > > > + .id = -1, > > > + .num_resources = ARRAY_SIZE(dove_cpufreq_resources), > > > + .resource = dove_cpufreq_resources, > > > +}; > > > + > > > +void __init dove_cpufreq_init(void) > > > +{ > > > + platform_device_register(&dove_cpufreq_device); > > > +} > > > > Please describe your power management unit in the DT. Your hardcoding > > physical addresses that can easily be described in the DT, and you don't > > appear to have an early probing requirement. > > Hi Mark > > We have been here before, with the kirkwood cpufreq driver. My v1 of > that driver had the cpufreq driver fully described in DT. My binding > got shot down and i was told that pseudo devices, like cpufreq, > cpuidle, should not be in DT, and a platform driver was the way to go. > So that is how kirkwood cpufreq works and dove follows that. > > Do you want to reverse that decision now? I'm not arguing for describing a pseudo-device. The power management unit is a real device. If we used that to register a cpufreq driver, it doesn't mean that we've registered a pseudo-device, but rather that we made a decision to register the cpufreq portions based on the necessary real devices being present. This code registers a platform device that has PMU register fields. We can easily describe the PMU in dt. The PMU _could_ be used by frameworks other than cpufreq, so describing it as a cpufreq device is wrong. Perhaps this was the problem with the proposed kirkwood binding? Thanks, Mark.
> I'm not arguing for describing a pseudo-device. The power management > unit is a real device. If we used that to register a cpufreq driver, it > doesn't mean that we've registered a pseudo-device, but rather that we > made a decision to register the cpufreq portions based on the necessary > real devices being present. Hi Mark I need a more concrete explanation to understand what you mean. Are you suggesting that i add a drivers/pmu/dove.c, which gets instantiated by DT? This driver would then instantiate the cpufreq driver? If so, it should also instantiate the gated clocks in drivers/clk/mvebu/dove.c, since they are also part of the PMU, according to the data sheet. The RTC and the thermal management unit are also part of the PMU. Should these drivers also be instantiated by the PMU somehow? > Perhaps this was the problem with the proposed kirkwood binding? Kirkwood does not have a PMU, at least not according to the data sheet. There are registers to control cpuidle, cpufreq, gating some clocks, the exact same RTC and a somewhat similar thermal sensor. However the data sheet makes no reference as to calling the collection the "PMU". Thanks Andrew
On Thu, Dec 05, 2013 at 07:29:39PM +0100, Andrew Lunn wrote: > > I'm not arguing for describing a pseudo-device. The power management > > unit is a real device. If we used that to register a cpufreq driver, it > > doesn't mean that we've registered a pseudo-device, but rather that we > > made a decision to register the cpufreq portions based on the necessary > > real devices being present. > > Hi Mark > > I need a more concrete explanation to understand what you mean. > > Are you suggesting that i add a drivers/pmu/dove.c, which gets > instantiated by DT? This driver would then instantiate the cpufreq > driver? If so, it should also instantiate the gated clocks in > drivers/clk/mvebu/dove.c, since they are also part of the PMU, > according to the data sheet. The RTC and the thermal management unit > are also part of the PMU. Should these drivers also be instantiated by > the PMU somehow? No, I don't think that is where Mark is heading. Separate the DT from Linux. Describe the PMU in DT. It's then up to Linux to figure out how to use (or not) that device. I don't know if we've encountered this scenario yet, but is it possible for Linux to have two drivers (cpufreq and cpuidle) answer to the same compatible string? Not that I'm advocating for that solution here... The only problem I see with that is locking on any resources both drivers use. I think we had a similar issue with watchdog/rtc. > > Perhaps this was the problem with the proposed kirkwood binding? > > Kirkwood does not have a PMU, at least not according to the data > sheet. There are registers to control cpuidle, cpufreq, gating some > clocks, the exact same RTC and a somewhat similar thermal > sensor. However the data sheet makes no reference as to calling the > collection the "PMU". If it quacks like a duck... :) thx, Jason.
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index c1f1a7eee953..908b02f8d901 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -500,6 +500,7 @@ config ARCH_IXP4XX config ARCH_DOVE bool "Marvell Dove" + select ARCH_HAS_CPUFREQ select ARCH_REQUIRE_GPIOLIB select CPU_PJ4 select GENERIC_CLOCKEVENTS diff --git a/arch/arm/mach-dove/board-dt.c b/arch/arm/mach-dove/board-dt.c index 49fa9abd09da..845e8b328432 100644 --- a/arch/arm/mach-dove/board-dt.c +++ b/arch/arm/mach-dove/board-dt.c @@ -27,6 +27,9 @@ static void __init dove_dt_init(void) tauros2_init(0); #endif BUG_ON(mvebu_mbus_dt_init()); + + dove_cpufreq_init(); + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); } diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c index c122bcff9f7c..323fcd4b27b6 100644 --- a/arch/arm/mach-dove/common.c +++ b/arch/arm/mach-dove/common.c @@ -344,6 +344,42 @@ void __init dove_sdio1_init(void) platform_device_register(&dove_sdio1); } +/***************************************************************************** + * CPU Frequency + ****************************************************************************/ +static struct resource dove_cpufreq_resources[] = { + [0] = { + .start = DOVE_PMU_PHYS_BASE, + .end = DOVE_PMU_PHYS_BASE + 0x7, + .flags = IORESOURCE_MEM, + .name = "cpufreq: DFS" + }, + [1] = { + .start = DOVE_PMU_PHYS_BASE + 0x8000, + .end = DOVE_PMU_PHYS_BASE + 0x8003, + .flags = IORESOURCE_MEM, + .name = "cpufreq: PMU CR" + }, + [2] = { + .start = DOVE_PMU_PHYS_BASE + 0x0044, + .end = DOVE_PMU_PHYS_BASE + 0x0047, + .flags = IORESOURCE_MEM, + .name = "cpufreq: PMU Clk Div" + }, +}; + +static struct platform_device dove_cpufreq_device = { + .name = "dove-cpufreq", + .id = -1, + .num_resources = ARRAY_SIZE(dove_cpufreq_resources), + .resource = dove_cpufreq_resources, +}; + +void __init dove_cpufreq_init(void) +{ + platform_device_register(&dove_cpufreq_device); +} + void __init dove_setup_cpu_wins(void) { /* diff --git a/arch/arm/mach-dove/common.h b/arch/arm/mach-dove/common.h index 1d725224d146..5c9a77bdd442 100644 --- a/arch/arm/mach-dove/common.h +++ b/arch/arm/mach-dove/common.h @@ -44,6 +44,7 @@ void dove_spi1_init(void); void dove_i2c_init(void); void dove_sdio0_init(void); void dove_sdio1_init(void); +void dove_cpufreq_init(void); void dove_restart(enum reboot_mode, const char *); #endif diff --git a/arch/arm/mach-dove/include/mach/dove.h b/arch/arm/mach-dove/include/mach/dove.h index 0c4b35f4ee5b..48db186ae6bc 100644 --- a/arch/arm/mach-dove/include/mach/dove.h +++ b/arch/arm/mach-dove/include/mach/dove.h @@ -144,6 +144,7 @@ #define DOVE_SD0_GPIO_SEL (1 << 0) /* Power Management */ +#define DOVE_PMU_PHYS_BASE (DOVE_SB_REGS_PHYS_BASE + 0xd0000) #define DOVE_PMU_VIRT_BASE (DOVE_SB_REGS_VIRT_BASE + 0xd0000) #define DOVE_PMU_SIG_CTRL (DOVE_PMU_VIRT_BASE + 0x802c)