Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V3] ARM: shmobile: Rework the PMIC IRQ line quirk
From: Geert Uytterhoeven @ 2018-06-13 11:28 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <3eda9da7-e26e-3e77-1040-9febe3b18abb@gmail.com>

Hi Marek,

On Mon, Jun 11, 2018 at 5:26 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 06/11/2018 04:30 PM, Geert Uytterhoeven wrote:
> > On Mon, Jun 11, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 06/11/2018 04:10 PM, Geert Uytterhoeven wrote:
> >>> On Mon, Jun 11, 2018 at 4:04 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 06/11/2018 03:49 PM, Geert Uytterhoeven wrote:
> >>>>> On Mon, Jun 11, 2018 at 3:39 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 06/11/2018 03:03 PM, Geert Uytterhoeven wrote:
> >>>>>>> On Mon, Jun 11, 2018 at 2:15 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> On 06/11/2018 11:56 AM, Geert Uytterhoeven wrote:
> >>>>>>>>> On Mon, Jun 4, 2018 at 7:59 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>> Rather than hard-coding the quirk topology, which stopped scaling,
> >>>>>>>>>> parse the information from DT. The code looks for all compatible
> >>>>>>>>>> PMICs -- da9036 and da9210 -- and checks if their IRQ line is tied
> >>>>>>>>>> to the same pin. If so, the code sends a matching sequence to the
> >>>>>>>>>> PMIC to deassert the IRQ.
> >>>>>
> >>>>>>>>>> +               ret = of_property_read_u32(np, "reg", &addr);
> >>>>>>>>>> +               if (ret)
> >>>>>>>>>> +                       return ret;
> >>>>>>>>>
> >>>>>>>>> I think it's safer to skip this entry and continue, after calling
> >>>>>>>>> kfree(quirk), of course.
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +               quirk->id = id;
> >>>>>>>>>> +               quirk->i2c_msg.addr = addr;
> >>>>>>>>>> +               quirk->shared = false;
> >>>>>>>>>> +
> >>>>>>>>>> +               ret = of_irq_parse_one(np, 0, &quirk->irq_args);
> >>>>>>>>>> +               if (ret)
> >>>>>>>>>> +                       return ret;
> >>>>>>>>>
> >>>>>>>>> kfree(quirk) and continue...
> >>>>>>>>
> >>>>>>>> I wonder if it shouldn't rather free the entire list and abort ?
> >>>>>>>
> >>>>>>> "Be strict when sending, be liberal when receiving."
> >>>>>>
> >>>>>> Meaning ? I think "the language barrier is protecting me" (TM)
> >>>>>
> >>>>> Do the best you can, given the buggy DT you received.
> >>>>> I.e. don't fail completely, just ignore the bad device node, and continue.
> >>>>
> >>>> But if you ignore node, you might as well ignore one which is shared and
> >>>> then the system crashes due to IRQ storm anyway. So hum, what can we do ?
> >>>
> >>> Correct. If it's a critical node, it will crash regardless.
> >>> If it's a non-critical node, you have the choice between aborting and crashing,
> >>> or ignoring and keeping the system alive. Your call.
> >>
> >> But wait, since we control which machines this code runs on , can't we
> >> assure they have valid DTs ? This situation with invalid DT starts to
> >> look a bit hypothetical to me.
> >
> > That assumes you keep the list of machines to check, and don't want to fix the
> > issue automatically when detected (on any R-Car Gen2 or RZ/G1 platform, so
> > you still need to check for r8a779[0-4] and r8a774[23457]).
>
> Yes, I want to keep a list of machines to check, to be _sure_ some
> machine doesn't randomly blow up.

Just checking for the presence of a "renesas,irqc" node should be sufficient.
Using that node would also get rid of the hardcoded IRQC_BASE address.
Note that the code assumes IRQ2. If another IRQ is used, that won't harm
much though (as in: if it didn't blow up before, it won't blow up now).

> > Anyway, as we care about booting old DTBs on new kernels (for a while), we
> > have a few more release cycles to bikeshed ;-)
>
> I was about to ask if this patch then makes any sense or not.

Sure. Less hard-coding is always better.
Especially if it means we can make it work on more machines automatically :-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* [PATCH 0/8] Add persistent clock support
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

We will meet below issues when compensating the suspend time for the timekeeping.

1. We have too many different ways of dealing with persistent timekeeping
across architectures, so it is hard for one driver to be compatible with
different architectures. For example, we should register register_persistent_clock()
on arm architecture, but we should set x86_platform.get_wallclock() on x86
architecture, and we should implement the read_persistent_clock64() on arm64
architecture and so on.

2. On some platforms (such as Spreadtrum platform), we registered the high
resolution timer as one clocksource to update the OS time, but the high
resolution timer will be stopped when system goes into suspend state.
So we use another one always-on timer (but low resolution) to calculate
the suspend time to compensate the OS time. Though we can register the
always-on timer as one clocksource, we need re-calculate the mult/shift
with one larger conversion range to calculate the suspend time and need
update the clock in case of running over the always-on timer. 

More duplicate code will be added if other platforms meet this case.

3. Now we have 3 sources that could be used to compensate the OS time:
Nonstop clocksource during suspend, persistent clock and rtc device,
which is complicated. Another hand is that the nonstop clocksource can
risk wrapping if the system suspend time is too long, so we need one
mechanism to wake up the system before the nonstop clocksource wrapping.

According to above issues, we can introduce one common persistent clock
framework to be compatible with different architectures, in future we will
remove the persistent clock implementation for each architecture. Also
this framework will implement common code to help drivers to register easily.
Moreover if we converted all SUSPEND_NONSTOP clocksource to register to
be one persistent clock, we can remove the SUSPEND_NONSTOP clocksource
accounting in timekeeping, which means we can only compensate the OS time
from persistent clock and RTC.

Will be appreciated for any comments. Thank you all.

Arnd posted some comments as below last time, but we did not get a general
consensus, so I post them again. Arnd said:

"I was planning to discuss this with Daniel and John during Linaro Connect,
but that didn't happen, so I'd like to bring up the bigger picture here again.

Today, we have a number of subsystem-type interfaces that deal with
time keeping in the wider sense (I might be missing some):
 - clock source
 - clock event
 - sched clock
 - real time clock
 - ptp clock
 - persistent clock

The first five have separate registration interfaces and may all refer
to different hardware blocks, or (more commonly) have some overlap
in the hardware. The fifth one is generalized by your series, without it
it's really architecture specific (as the other ones were one one point).

Are we happy with that structure in the long run? One of my earlier
comments on this series was that I thought it should be combined with
the clocksource registration, but upon talking to Baolin about it more,
I realized that this just follows the same structure that we have for the
others.

In theory, we could have a more abstract way of registering a clock
hardware that interfaces with any combination of the six subsystems
I mentioned above, with a superset of the subsystem specific structures
and a set of flags that indicate what a particular device is usable for.

Combining all six might be a bit too much (in particular rtc, though
it clearly overlaps the persistent-clk case), but what your general
ideas on where we should be heading? Is it worth reworking the
core kernel portion of the subsystems to simplify the individual
drivers?"

John also posted some important comments for this patch set, that I quite
agree with his points. John said:

"For context, these abstractions have grown out of the need for using
different hardware components for all of these. It was quite common
for x86 hardware to use the acpi_pm for clocksource, lapic/PIT for
clockevent, tsc for sched_clock and CMOS RTC for persistent clock.
While some of these could be backed by the same hardware, it wasn't
common. However, hardware with less restrictions have allowed in some
cases for these to be more unified, but I'm not sure if its particularly common.

Another part of the reason that we don't combine the above
abstractions, even when they are backed by the same hardware, is
because some of the fields used for freq conversion (mult/shift) have
different needs for the different types of accounting.

For instance, with a clocksource, we are very focused on avoiding
error to keep timekeeing accurate, thus we want to use as large a
shift (and thus mult) as possible (and do our shifting as late as
possible in our accounting). However, that then shrinks the amount of
time that can be accumulated in one go w/o causing an overflow.

Where as with sched_clock, we don't worry as much as about accuracy,
so we can use smaller shifts (and thus mults), and thus can go for
longer periods of time between accumulating without worrying.

Similarly for the persistent clock case we don't need need to worry as
much about accuracy, so we can can use smaller shifts, but we are not
in as much of a hot patch, so we can also"

Changes since RFC V1:
 - Move the alarmtimer starting into alarmtimer.c.
 - Export persistent_clock_start_alarmtimer().
 - Add one memeber to indicate if the alarmtimer was initialized.
 - Fix build issues.

Baolin Wang (8):
  time: Add persistent clock support
  clocksource: sprd: Add one persistent timer for Spreadtrum platform
  arm: time: Remove the persistent clock support for ARM architecture
  clocksource: arm_arch_timer: Register the persistent clock
  clocksource: timer-ti-32k: Register the persistent clock
  clocksource: time-pistachio: Register the persistent clock
  x86: tsc: Register the persistent clock
  time: timekeeping: Remove time compensating from nonstop clocksources

 arch/arm/include/asm/mach/time.h     |    4 -
 arch/arm/kernel/time.c               |   36 -------
 arch/arm/plat-omap/Kconfig           |    1 +
 arch/arm/plat-omap/counter_32k.c     |   44 ++------
 arch/x86/Kconfig                     |    1 +
 arch/x86/kernel/tsc.c                |   21 ++++
 drivers/clocksource/Kconfig          |    4 +
 drivers/clocksource/arm_arch_timer.c |   10 ++
 drivers/clocksource/tegra20_timer.c  |   12 ++-
 drivers/clocksource/time-pistachio.c |    3 +
 drivers/clocksource/timer-sprd.c     |   80 +++++++++++++++
 drivers/clocksource/timer-ti-32k.c   |    4 +
 include/linux/persistent_clock.h     |   23 +++++
 kernel/time/Kconfig                  |    4 +
 kernel/time/Makefile                 |    1 +
 kernel/time/alarmtimer.c             |    4 +
 kernel/time/persistent_clock.c       |  184 ++++++++++++++++++++++++++++++++++
 kernel/time/timekeeping.c            |   19 +---
 18 files changed, 360 insertions(+), 95 deletions(-)
 create mode 100644 include/linux/persistent_clock.h
 create mode 100644 kernel/time/persistent_clock.c

-- 
1.7.9.5

^ permalink raw reply

* [PATCH 1/8] time: Add persistent clock support
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

On our Spreadtrum SC9860 platform, we registered the high resolution
ARM generic timer as one clocksource to update the OS time, but the
ARM generic timer will be stopped in suspend state. So we use one 64bit
always-on timer (but low resolution) of Spreadtrum to calculate the
suspend time to compensate the OS time. Though we can register the
always-on timer as one clocksource, we need re-calculate the
mult/shift with one larger conversion range to calculate the suspend
time.

But now we have too many different ways of dealing with persistent
timekeeping across architectures, and there will be many duplicate
code if we register one timer to be one persistent clock. Thus it
will be more helpful if we add one common framework for timer drivers
to be registered as one persistent clock and implement the common
read_persistent_clock64() to compensate the OS time.

Moreover we can register the clocksource with CLOCK_SOURCE_SUSPEND_NONSTOP
to be one persistent clock, then we can simplify the suspend/resume
accounting by removing CLOCK_SOURCE_SUSPEND_NONSTOP timing. After that
we can only compensate the OS time by persistent clock or RTC.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/linux/persistent_clock.h |   23 +++++
 kernel/time/Kconfig              |    4 +
 kernel/time/Makefile             |    1 +
 kernel/time/alarmtimer.c         |    4 +
 kernel/time/persistent_clock.c   |  184 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 216 insertions(+)
 create mode 100644 include/linux/persistent_clock.h
 create mode 100644 kernel/time/persistent_clock.c

diff --git a/include/linux/persistent_clock.h b/include/linux/persistent_clock.h
new file mode 100644
index 0000000..7d42c1a
--- /dev/null
+++ b/include/linux/persistent_clock.h
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef __PERSISTENT_CLOCK_H__
+#define __PERSISTENT_CLOCK_H__
+
+#ifdef CONFIG_PERSISTENT_CLOCK
+extern int persistent_clock_init_and_register(u64 (*read)(void),
+					      u64 mask, u32 freq,
+					      u64 maxsec);
+extern void persistent_clock_cleanup(void);
+extern void persistent_clock_start_alarmtimer(void);
+#else
+static inline int persistent_clock_init_and_register(u64 (*read)(void),
+						     u64 mask, u32 freq,
+						     u64 maxsec)
+{
+	return 0;
+}
+
+static inline void persistent_clock_cleanup(void) { }
+static inline void persistent_clock_start_alarmtimer(void) { }
+#endif
+
+#endif
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index 78eabc4..7188600 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -47,6 +47,10 @@ config GENERIC_CLOCKEVENTS_MIN_ADJUST
 config GENERIC_CMOS_UPDATE
 	bool
 
+# Persistent clock support
+config PERSISTENT_CLOCK
+	bool
+
 if GENERIC_CLOCKEVENTS
 menu "Timers subsystem"
 
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index f1e46f3..f6d368f 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_GENERIC_SCHED_CLOCK)		+= sched_clock.o
 obj-$(CONFIG_TICK_ONESHOT)			+= tick-oneshot.o tick-sched.o
 obj-$(CONFIG_DEBUG_FS)				+= timekeeping_debug.o
 obj-$(CONFIG_TEST_UDELAY)			+= test_udelay.o
+obj-$(CONFIG_PERSISTENT_CLOCK)			+= persistent_clock.o
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 639321b..1518fdb 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -29,6 +29,7 @@
 #include <linux/freezer.h>
 #include <linux/compat.h>
 #include <linux/module.h>
+#include <linux/persistent_clock.h>
 
 #include "posix-timers.h"
 
@@ -892,6 +893,9 @@ static int __init alarmtimer_init(void)
 		error = PTR_ERR(pdev);
 		goto out_drv;
 	}
+
+	/* Start one alarmtimer to update persistent clock */
+	persistent_clock_start_alarmtimer();
 	return 0;
 
 out_drv:
diff --git a/kernel/time/persistent_clock.c b/kernel/time/persistent_clock.c
new file mode 100644
index 0000000..edaa659
--- /dev/null
+++ b/kernel/time/persistent_clock.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Linaro, Inc.
+ *
+ * Author: Baolin Wang <baolin.wang@linaro.org>
+ */
+
+#include <linux/alarmtimer.h>
+#include <linux/clocksource.h>
+#include <linux/persistent_clock.h>
+
+/**
+ * persistent_clock_read_data - data required to read persistent clock
+ * @read: Returns a cycle value from persistent clock.
+ * @last_cycles: Clock cycle value at last update.
+ * @last_ns: Time value (nanoseconds) at last update.
+ * @mask: Bitmask for two's complement subtraction of non 64bit clocks.
+ * @mult: Cycle to nanosecond multiplier.
+ * @shift: Cycle to nanosecond divisor.
+ */
+struct persistent_clock_read_data {
+	u64 (*read)(void);
+	u64 last_cycles;
+	u64 last_ns;
+	u64 mask;
+	u32 mult;
+	u32 shift;
+};
+
+/**
+ * persistent_clock - represent the persistent clock
+ * @read_data: Data required to read from persistent clock.
+ * @seq: Sequence counter for protecting updates.
+ * @freq: The frequency of the persistent clock.
+ * @wrap: Duration for persistent clock can run before wrapping.
+ * @alarm: Update timeout for persistent clock wrap.
+ * @alarm_inited: Indicate if the alarm has been initialized.
+ */
+struct persistent_clock {
+	struct persistent_clock_read_data read_data;
+	seqcount_t seq;
+	u32 freq;
+	ktime_t wrap;
+	struct alarm alarm;
+	bool alarm_inited;
+};
+
+static struct persistent_clock p;
+
+void read_persistent_clock64(struct timespec64 *ts)
+{
+	struct persistent_clock_read_data *read_data = &p.read_data;
+	unsigned long seq;
+	u64 delta, nsecs;
+
+	if (!read_data->read) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+		return;
+	}
+
+	do {
+		seq = read_seqcount_begin(&p.seq);
+		delta = (read_data->read() - read_data->last_cycles) &
+			read_data->mask;
+
+		nsecs = read_data->last_ns +
+			clocksource_cyc2ns(delta, read_data->mult,
+					   read_data->shift);
+		*ts = ns_to_timespec64(nsecs);
+	} while (read_seqcount_retry(&p.seq, seq));
+}
+
+static void persistent_clock_update(void)
+{
+	struct persistent_clock_read_data *read_data = &p.read_data;
+	u64 cycles, delta;
+
+	write_seqcount_begin(&p.seq);
+
+	cycles = read_data->read();
+	delta = (cycles - read_data->last_cycles) & read_data->mask;
+	read_data->last_ns += clocksource_cyc2ns(delta, read_data->mult,
+						 read_data->shift);
+	read_data->last_cycles = cycles;
+
+	write_seqcount_end(&p.seq);
+}
+
+static enum alarmtimer_restart persistent_clock_alarm_fired(struct alarm *alarm,
+							    ktime_t now)
+{
+	persistent_clock_update();
+
+	alarm_forward(&p.alarm, now, p.wrap);
+	return ALARMTIMER_RESTART;
+}
+
+int persistent_clock_init_and_register(u64 (*read)(void), u64 mask,
+				       u32 freq, u64 maxsec)
+{
+	struct persistent_clock_read_data *read_data = &p.read_data;
+	u64 wrap, res, secs = maxsec;
+
+	if (!read || !mask || !freq)
+		return -EINVAL;
+
+	if (!secs) {
+		/*
+		 * If the timer driver did not specify the maximum conversion
+		 * seconds of the persistent clock, then we calculate the
+		 * conversion range with the persistent clock's bits and
+		 * frequency.
+		 */
+		secs = mask;
+		do_div(secs, freq);
+
+		/*
+		 * Some persistent counter can be larger than 32bit, so we
+		 * need limit the max suspend time to have a good conversion
+		 * precision. So 24 hours may be enough usually.
+		 */
+		if (secs > 86400)
+			secs = 86400;
+	}
+
+	/* Calculate the mult/shift to convert cycles to ns. */
+	clocks_calc_mult_shift(&read_data->mult, &read_data->shift, freq,
+			       NSEC_PER_SEC, (u32)secs);
+
+	/* Calculate how many nanoseconds until we risk wrapping. */
+	wrap = clocks_calc_max_nsecs(read_data->mult, read_data->shift, 0,
+				     mask, NULL);
+	p.wrap = ns_to_ktime(wrap);
+
+	p.freq = freq;
+	read_data->mask = mask;
+	read_data->read = read;
+
+	persistent_clock_update();
+
+	/* Calculate the ns resolution of this persistent clock. */
+	res = clocksource_cyc2ns(1ULL, read_data->mult, read_data->shift);
+
+	pr_info("persistent clock: mask %llu at %uHz, resolution %lluns, wraps every %lluns\n",
+		mask, freq, res, wrap);
+	return 0;
+}
+
+void persistent_clock_cleanup(void)
+{
+	p.read_data.read = NULL;
+
+	if (p.alarm_inited) {
+		alarm_cancel(&p.alarm);
+		p.alarm_inited = false;
+	}
+}
+
+void persistent_clock_start_alarmtimer(void)
+{
+	struct persistent_clock_read_data *read_data = &p.read_data;
+	ktime_t now;
+
+	/*
+	 * If no persistent clock function has been provided or the alarmtimer
+	 * has been initialized at that point, just return.
+	 */
+	if (!read_data->read || p.alarm_inited)
+		return;
+
+	persistent_clock_update();
+
+	/*
+	 * Since the persistent clock will not be stopped when system enters the
+	 * suspend state, thus we need start one alarmtimer to wakeup the system
+	 * to update the persistent clock before wrapping. We should start the
+	 * update alarmtimer after the alarmtimer subsystem was initialized.
+	 */
+	alarm_init(&p.alarm, ALARM_BOOTTIME, persistent_clock_alarm_fired);
+	now = ktime_get_boottime();
+	alarm_start(&p.alarm, ktime_add(now, p.wrap));
+	p.alarm_inited = true;
+}
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 2/8] clocksource: sprd: Add one persistent timer for Spreadtrum platform
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

On Spreadtrum SC9860 platform, we need one persistent timer to calculate
the suspend time to compensate the OS time.

This patch registers one Spreadtrum AON timer as persistent timer, which
runs at 32bit and periodic mode.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/clocksource/Kconfig      |    1 +
 drivers/clocksource/timer-sprd.c |   80 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index dec0dd8..7f11c6c 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -455,6 +455,7 @@ config SPRD_TIMER
 	depends on (ARCH_SPRD || COMPILE_TEST)
 	default ARCH_SPRD
 	select TIMER_OF
+	select PERSISTENT_CLOCK
 	help
 	  Enables support for the Spreadtrum timer driver.
 
diff --git a/drivers/clocksource/timer-sprd.c b/drivers/clocksource/timer-sprd.c
index ef9ebea..c6f657a 100644
--- a/drivers/clocksource/timer-sprd.c
+++ b/drivers/clocksource/timer-sprd.c
@@ -3,8 +3,11 @@
  * Copyright (C) 2017 Spreadtrum Communications Inc.
  */
 
+#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/persistent_clock.h>
 
 #include "timer-of.h"
 
@@ -157,3 +160,80 @@ static int __init sprd_timer_init(struct device_node *np)
 }
 
 TIMER_OF_DECLARE(sc9860_timer, "sprd,sc9860-timer", sprd_timer_init);
+
+void __iomem *pbase;
+
+static u64 sprd_persistent_timer_read(void)
+{
+	return ~(u64)readl_relaxed(pbase + TIMER_VALUE_SHDW_LO) &
+		CLOCKSOURCE_MASK(32);
+}
+
+static void sprd_persistent_timer_disable(void)
+{
+	sprd_timer_disable(pbase);
+}
+
+static void sprd_persistent_timer_enable(void)
+{
+	sprd_timer_disable(pbase);
+	sprd_timer_update_counter(pbase, TIMER_VALUE_LO_MASK);
+	sprd_timer_enable(pbase, TIMER_CTL_PERIOD_MODE);
+}
+
+static int __init sprd_persistent_timer_init(struct device_node *np)
+{
+	struct clk *clk;
+	u32 freq;
+	int ret;
+
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk)) {
+		pr_err("Can't get timer clock for %pOF\n", np);
+		return PTR_ERR(clk);
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("Failed to enable clock for %pOF\n", np);
+		clk_put(clk);
+		return ret;
+	}
+
+	freq = clk_get_rate(clk);
+	if (!freq) {
+		pr_err("Failed to get clock rate for %pOF\n", np);
+		ret = -EINVAL;
+		goto clk_rate_err;
+	}
+
+	pbase = of_io_request_and_map(np, 0, of_node_full_name(np));
+	if (IS_ERR(pbase)) {
+		pr_err("Can't map timer registers for %pOF\n", np);
+		ret = PTR_ERR(pbase);
+		goto clk_rate_err;
+	}
+
+	sprd_persistent_timer_enable();
+
+	ret = persistent_clock_init_and_register(sprd_persistent_timer_read,
+						 CLOCKSOURCE_MASK(32), freq, 0);
+	if (ret) {
+		pr_err("Failed to register persistent clock for %pOF\n", np);
+		goto persist_err;
+	}
+
+	return 0;
+
+persist_err:
+	sprd_persistent_timer_disable();
+	iounmap(pbase);
+clk_rate_err:
+	clk_disable_unprepare(clk);
+	clk_put(clk);
+
+	return ret;
+}
+
+TIMER_OF_DECLARE(sc9860_persistent_timer, "sprd,sc9860-persistent-timer",
+		 sprd_persistent_timer_init);
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 3/8] arm: time: Remove the persistent clock support for ARM architecture
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

We have introduced the persistent clock framework to support the OS time
compensating from persistent clock, and we will convert all drivers to
use common persistent clock framework instead of the persistent clock
support used only for the ARM architecture. So we can remove these code
with converting the Omap 32k counter and tegra20 timer.

Moreover there are no drivers will register read_boot_clock64(), so
remove it too.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 arch/arm/include/asm/mach/time.h    |    4 ----
 arch/arm/kernel/time.c              |   36 ----------------------------
 arch/arm/plat-omap/Kconfig          |    1 +
 arch/arm/plat-omap/counter_32k.c    |   44 ++++++-----------------------------
 drivers/clocksource/tegra20_timer.c |   12 +++++++---
 5 files changed, 17 insertions(+), 80 deletions(-)

diff --git a/arch/arm/include/asm/mach/time.h b/arch/arm/include/asm/mach/time.h
index 0f79e4d..3cbcafc 100644
--- a/arch/arm/include/asm/mach/time.h
+++ b/arch/arm/include/asm/mach/time.h
@@ -12,8 +12,4 @@
 
 extern void timer_tick(void);
 
-typedef void (*clock_access_fn)(struct timespec64 *);
-extern int register_persistent_clock(clock_access_fn read_boot,
-				     clock_access_fn read_persistent);
-
 #endif
diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index cf2701c..713905c 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -76,42 +76,6 @@ void timer_tick(void)
 }
 #endif
 
-static void dummy_clock_access(struct timespec64 *ts)
-{
-	ts->tv_sec = 0;
-	ts->tv_nsec = 0;
-}
-
-static clock_access_fn __read_persistent_clock = dummy_clock_access;
-static clock_access_fn __read_boot_clock = dummy_clock_access;
-
-void read_persistent_clock64(struct timespec64 *ts)
-{
-	__read_persistent_clock(ts);
-}
-
-void read_boot_clock64(struct timespec64 *ts)
-{
-	__read_boot_clock(ts);
-}
-
-int __init register_persistent_clock(clock_access_fn read_boot,
-				     clock_access_fn read_persistent)
-{
-	/* Only allow the clockaccess functions to be registered once */
-	if (__read_persistent_clock == dummy_clock_access &&
-	    __read_boot_clock == dummy_clock_access) {
-		if (read_boot)
-			__read_boot_clock = read_boot;
-		if (read_persistent)
-			__read_persistent_clock = read_persistent;
-
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
 void __init time_init(void)
 {
 	if (machine_desc->init_time) {
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index c0a242c..073a80f 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 config ARCH_OMAP
+	select PERSISTENT_CLOCK
 	bool
 
 if ARCH_OMAP
diff --git a/arch/arm/plat-omap/counter_32k.c b/arch/arm/plat-omap/counter_32k.c
index 2438b96..5d52f7c 100644
--- a/arch/arm/plat-omap/counter_32k.c
+++ b/arch/arm/plat-omap/counter_32k.c
@@ -19,8 +19,7 @@
 #include <linux/io.h>
 #include <linux/clocksource.h>
 #include <linux/sched_clock.h>
-
-#include <asm/mach/time.h>
+#include <linux/persistent_clock.h>
 
 #include <plat/counter-32k.h>
 
@@ -44,33 +43,6 @@ static u64 notrace omap_32k_read_sched_clock(void)
 }
 
 /**
- * omap_read_persistent_clock64 -  Return time from a persistent clock.
- *
- * Reads the time from a source which isn't disabled during PM, the
- * 32k sync timer.  Convert the cycles elapsed since last read into
- * nsecs and adds to a monotonically increasing timespec64.
- */
-static struct timespec64 persistent_ts;
-static cycles_t cycles;
-static unsigned int persistent_mult, persistent_shift;
-
-static void omap_read_persistent_clock64(struct timespec64 *ts)
-{
-	unsigned long long nsecs;
-	cycles_t last_cycles;
-
-	last_cycles = cycles;
-	cycles = sync32k_cnt_reg ? readl_relaxed(sync32k_cnt_reg) : 0;
-
-	nsecs = clocksource_cyc2ns(cycles - last_cycles,
-					persistent_mult, persistent_shift);
-
-	timespec64_add_ns(&persistent_ts, nsecs);
-
-	*ts = persistent_ts;
-}
-
-/**
  * omap_init_clocksource_32k - setup and register counter 32k as a
  * kernel clocksource
  * @pbase: base addr of counter_32k module
@@ -95,13 +67,6 @@ int __init omap_init_clocksource_32k(void __iomem *vbase)
 	else
 		sync32k_cnt_reg = vbase + OMAP2_32KSYNCNT_CR_OFF_LOW;
 
-	/*
-	 * 120000 rough estimate from the calculations in
-	 * __clocksource_update_freq_scale.
-	 */
-	clocks_calc_mult_shift(&persistent_mult, &persistent_shift,
-			32768, NSEC_PER_SEC, 120000);
-
 	ret = clocksource_mmio_init(sync32k_cnt_reg, "32k_counter", 32768,
 				250, 32, clocksource_mmio_readl_up);
 	if (ret) {
@@ -110,7 +75,12 @@ int __init omap_init_clocksource_32k(void __iomem *vbase)
 	}
 
 	sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
-	register_persistent_clock(NULL, omap_read_persistent_clock64);
+	/*
+	 * 120000 rough estimate from the calculations in
+	 * __clocksource_update_freq_scale.
+	 */
+	persistent_clock_init_and_register(omap_32k_read_sched_clock,
+					   CLOCKSOURCE_MASK(32), 32768, 120000);
 	pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n");
 
 	return 0;
diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index c337a81..97a34cb 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -124,7 +124,7 @@ static u64 tegra_rtc_read_ms(void)
 }
 
 /*
- * tegra_read_persistent_clock64 -  Return time from a persistent clock.
+ * read_persistent_clock64 -  Return time from a persistent clock.
  *
  * Reads the time from a source which isn't disabled during PM, the
  * 32k sync timer.  Convert the cycles elapsed since last read into
@@ -133,10 +133,16 @@ static u64 tegra_rtc_read_ms(void)
  * tegra_rtc driver could be executing to avoid race conditions
  * on the RTC shadow register
  */
-static void tegra_read_persistent_clock64(struct timespec64 *ts)
+void read_persistent_clock64(struct timespec64 *ts)
 {
 	u64 delta;
 
+	if (!rtc_base) {
+		ts->tv_sec = 0;
+		ts->tv_nsec = 0;
+		return;
+	}
+
 	last_persistent_ms = persistent_ms;
 	persistent_ms = tegra_rtc_read_ms();
 	delta = persistent_ms - last_persistent_ms;
@@ -259,6 +265,6 @@ static int __init tegra20_init_rtc(struct device_node *np)
 	else
 		clk_prepare_enable(clk);
 
-	return register_persistent_clock(NULL, tegra_read_persistent_clock64);
+	return 0;
 }
 TIMER_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 4/8] clocksource: arm_arch_timer: Register the persistent clock
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

Register the persistent clock to compensate the suspend time for OS time,
if the ARM counter clocksource will not be stopped in suspend state.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/clocksource/Kconfig          |    1 +
 drivers/clocksource/arm_arch_timer.c |   10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 7f11c6c..5e51fcf 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -308,6 +308,7 @@ config ARC_TIMERS_64BIT
 
 config ARM_ARCH_TIMER
 	bool
+	select PERSISTENT_CLOCK
 	select TIMER_OF if OF
 	select TIMER_ACPI if ACPI
 
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 57cb2f0..671be63 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -32,6 +32,7 @@
 #include <asm/virt.h>
 
 #include <clocksource/arm_arch_timer.h>
+#include <linux/persistent_clock.h>
 
 #undef pr_fmt
 #define pr_fmt(fmt) "arch_timer: " fmt
@@ -950,6 +951,15 @@ static void __init arch_counter_register(unsigned type)
 
 	/* 56 bits minimum, so we assume worst case rollover */
 	sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);
+
+	/*
+	 * Register the persistent clock if the clocksource will not be stopped
+	 * in suspend state.
+	 */
+	if (!arch_counter_suspend_stop)
+		persistent_clock_init_and_register(arch_timer_read_counter,
+						   CLOCKSOURCE_MASK(56),
+						   arch_timer_rate, 0);
 }
 
 static void arch_timer_stop(struct clock_event_device *clk)
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 5/8] clocksource: timer-ti-32k: Register the persistent clock
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

Since the 32K counter is always available, then we can register the
persistent clock to compensate the suspend time for the OS time.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/clocksource/Kconfig        |    1 +
 drivers/clocksource/timer-ti-32k.c |    4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5e51fcf..3cd136f 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -262,6 +262,7 @@ config CLKSRC_TI_32K
 	bool "Texas Instruments 32.768 Hz Clocksource" if COMPILE_TEST
 	depends on GENERIC_SCHED_CLOCK
 	select TIMER_OF if OF
+	select PERSISTENT_CLOCK
 	help
 	  This option enables support for Texas Instruments 32.768 Hz clocksource
 	  available on many OMAP-like platforms.
diff --git a/drivers/clocksource/timer-ti-32k.c b/drivers/clocksource/timer-ti-32k.c
index 880a861..353ff9d 100644
--- a/drivers/clocksource/timer-ti-32k.c
+++ b/drivers/clocksource/timer-ti-32k.c
@@ -41,6 +41,7 @@
 #include <linux/clocksource.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/persistent_clock.h>
 
 /*
  * 32KHz clocksource ... always available, on pretty most chips except
@@ -120,6 +121,9 @@ static int __init ti_32k_timer_init(struct device_node *np)
 	}
 
 	sched_clock_register(omap_32k_read_sched_clock, 32, 32768);
+	persistent_clock_init_and_register(omap_32k_read_sched_clock,
+					   CLOCKSOURCE_MASK(32), 32768, 0);
+
 	pr_info("OMAP clocksource: 32k_counter at 32768 Hz\n");
 
 	return 0;
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 6/8] clocksource: time-pistachio: Register the persistent clock
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

Since the timer on pistachio platform is always available, we can
register it as one persistent clock to compensate the suspend time
for the OS time.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/clocksource/Kconfig          |    1 +
 drivers/clocksource/time-pistachio.c |    3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 3cd136f..af552ba 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -255,6 +255,7 @@ config CLKSRC_PISTACHIO
 	bool "Clocksource for Pistachio SoC" if COMPILE_TEST
 	depends on HAS_IOMEM
 	select TIMER_OF
+	select PERSISTENT_CLOCK
 	help
 	  Enables the clocksource for the Pistachio SoC.
 
diff --git a/drivers/clocksource/time-pistachio.c b/drivers/clocksource/time-pistachio.c
index a2dd85d..5c3eb71 100644
--- a/drivers/clocksource/time-pistachio.c
+++ b/drivers/clocksource/time-pistachio.c
@@ -20,6 +20,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/persistent_clock.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/sched_clock.h>
@@ -212,6 +213,8 @@ static int __init pistachio_clksrc_of_init(struct device_node *node)
 
 	raw_spin_lock_init(&pcs_gpt.lock);
 	sched_clock_register(pistachio_read_sched_clock, 32, rate);
+	persistent_clock_init_and_register(pistachio_read_sched_clock,
+					   CLOCKSOURCE_MASK(32), rate, 0);
 	return clocksource_register_hz(&pcs_gpt.cs, rate);
 }
 TIMER_OF_DECLARE(pistachio_gptimer, "img,pistachio-gptimer",
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 7/8] x86: tsc: Register the persistent clock
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

Register the tsc as one persistent clock to compensate the suspend time
if the tsc clocksource is always available.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 arch/x86/Kconfig      |    1 +
 arch/x86/kernel/tsc.c |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 297789a..549dd01 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -200,6 +200,7 @@ config X86
 	select USER_STACKTRACE_SUPPORT
 	select VIRT_TO_BUS
 	select X86_FEATURE_NAMES		if PROC_FS
+	select PERSISTENT_CLOCK
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..cb4f495 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -11,6 +11,7 @@
 #include <linux/delay.h>
 #include <linux/clocksource.h>
 #include <linux/percpu.h>
+#include <linux/persistent_clock.h>
 #include <linux/timex.h>
 #include <linux/static_key.h>
 
@@ -1032,6 +1033,11 @@ static u64 read_tsc(struct clocksource *cs)
 	return (u64)rdtsc_ordered();
 }
 
+static u64 notrace tsc_read_persistent_clock(void)
+{
+	return (u64)rdtsc_ordered();
+}
+
 static void tsc_cs_mark_unstable(struct clocksource *cs)
 {
 	if (tsc_unstable)
@@ -1300,6 +1306,14 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 	if (boot_cpu_has(X86_FEATURE_ART))
 		art_related_clocksource = &clocksource_tsc;
 	clocksource_register_khz(&clocksource_tsc, tsc_khz);
+
+	if (clocksource_tsc.flags & CLOCK_SOURCE_SUSPEND_NONSTOP) {
+		persistent_clock_init_and_register(tsc_read_persistent_clock,
+						   CLOCKSOURCE_MASK(64),
+						   tsc_khz * 1000, 0);
+		persistent_clock_start_alarmtimer();
+	}
+
 unreg:
 	clocksource_unregister(&clocksource_tsc_early);
 }
@@ -1327,6 +1341,13 @@ static int __init init_tsc_clocksource(void)
 		if (boot_cpu_has(X86_FEATURE_ART))
 			art_related_clocksource = &clocksource_tsc;
 		clocksource_register_khz(&clocksource_tsc, tsc_khz);
+
+		if (clocksource_tsc.flags & CLOCK_SOURCE_SUSPEND_NONSTOP) {
+			persistent_clock_init_and_register(tsc_read_persistent_clock,
+							   CLOCKSOURCE_MASK(64),
+							   tsc_khz * 1000, 0);
+			persistent_clock_start_alarmtimer();
+		}
 unreg:
 		clocksource_unregister(&clocksource_tsc_early);
 		return 0;
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH 8/8] time: timekeeping: Remove time compensating from nonstop clocksources
From: Baolin Wang @ 2018-06-13 11:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528878545.git.baolin.wang@linaro.org>

Since we have converted all nonstop clocksources to use persistent clock,
thus we can simplify the time compensating by removing the nonstop
clocksources. Now we can compensate the suspend time for the OS time from
the persistent clock or rtc device.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 kernel/time/timekeeping.c |   19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4786df9..3026d98 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1666,7 +1666,6 @@ void timekeeping_inject_sleeptime64(struct timespec64 *delta)
 void timekeeping_resume(void)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
-	struct clocksource *clock = tk->tkr_mono.clock;
 	unsigned long flags;
 	struct timespec64 ts_new, ts_delta;
 	u64 cycle_now;
@@ -1682,27 +1681,17 @@ void timekeeping_resume(void)
 
 	/*
 	 * After system resumes, we need to calculate the suspended time and
-	 * compensate it for the OS time. There are 3 sources that could be
-	 * used: Nonstop clocksource during suspend, persistent clock and rtc
-	 * device.
+	 * compensate it for the OS time. There are 2 sources that could be
+	 * used: persistent clock and rtc device.
 	 *
 	 * One specific platform may have 1 or 2 or all of them, and the
 	 * preference will be:
-	 *	suspend-nonstop clocksource -> persistent clock -> rtc
+	 *	persistent clock -> rtc
 	 * The less preferred source will only be tried if there is no better
 	 * usable source. The rtc part is handled separately in rtc core code.
 	 */
 	cycle_now = tk_clock_read(&tk->tkr_mono);
-	if ((clock->flags & CLOCK_SOURCE_SUSPEND_NONSTOP) &&
-		cycle_now > tk->tkr_mono.cycle_last) {
-		u64 nsec, cyc_delta;
-
-		cyc_delta = clocksource_delta(cycle_now, tk->tkr_mono.cycle_last,
-					      tk->tkr_mono.mask);
-		nsec = mul_u64_u32_shr(cyc_delta, clock->mult, clock->shift);
-		ts_delta = ns_to_timespec64(nsec);
-		sleeptime_injected = true;
-	} else if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
+	if (timespec64_compare(&ts_new, &timekeeping_suspend_time) > 0) {
 		ts_delta = timespec64_sub(ts_new, timekeeping_suspend_time);
 		sleeptime_injected = true;
 	}
-- 
1.7.9.5

^ permalink raw reply related

* [GIT PULL] Renesas ARM Based SoC Fixes for v4.18
From: Simon Horman @ 2018-06-13 11:41 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Olof, Hi Kevin, Hi Arnd,

Please consider these Renesas ARM based SoC fixes for v4.18.

This pull request is based on renesas-soc-for-v4.18,
which you have already pulled.

The following changes since commit 086b399965a7ee7e50c3b3c57f2dba30ff0334b0:

  soc: renesas: r8a77990-sysc: Add workaround for 3DG-{A,B} (2018-05-16 10:57:44 +0200)

are available in the git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git tags/renesas-fixes-for-v4.18

for you to fetch changes up to 977d5ba4507dfe5b1346597ee57750262d8d2b19:

  soc: renesas: rcar-sysc: Make PM domain initialization more robust (2018-06-08 10:04:25 +0200)

----------------------------------------------------------------
Renesas ARM Based SoC Fixes for v4.18

Make PM domain initialization more robust in Renesas R-Car SYSC driver.
This resolves a regression due to re-parenting of PM domains by
086b399965a7ee7e ("soc: renesas: r8a77990-sysc: Add workaround for 3DG-{A,B}").

----------------------------------------------------------------
Geert Uytterhoeven (1):
      soc: renesas: rcar-sysc: Make PM domain initialization more robust

 drivers/soc/renesas/rcar-sysc.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

^ permalink raw reply

* [PATCH 1/1] soc: renesas: rcar-sysc: Make PM domain initialization more robust
From: Simon Horman @ 2018-06-13 11:41 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <cover.1528889592.git.horms+renesas@verge.net.au>

From: Geert Uytterhoeven <geert+renesas@glider.be>

The quirk for R-Car E3 ES1.0 added in commit 086b399965a7ee7e ("soc:
renesas: r8a77990-sysc: Add workaround for 3DG-{A,B}") makes the 3DG-A
PM domain a subdomain of the 3DG-B PM domain.  However, registering
3DG-A with its parent fails silently, as the 3DG-B PM domain hasn't been
registered yet, and such failures are never reported.

Fix this by:
  1. Splitting PM Domain initialization in two steps, so all PM domains
     are registered before any child-parent links are established,
  2. Reporting any failures in establishing child-parent relations.

Check for and report pm_genpd_init() failures, too, as that function
gained a return value in commit 7eb231c337e00735 ("PM / Domains: Convert
pm_genpd_init() to return an error code").

Fixes: 086b399965a7ee7e ("soc: renesas: r8a77990-sysc: Add workaround for 3DG-{A,B}")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/soc/renesas/rcar-sysc.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
index 95120acc4d80..50d03d8b4f9a 100644
--- a/drivers/soc/renesas/rcar-sysc.c
+++ b/drivers/soc/renesas/rcar-sysc.c
@@ -194,11 +194,12 @@ static int rcar_sysc_pd_power_on(struct generic_pm_domain *genpd)
 
 static bool has_cpg_mstp;
 
-static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
+static int __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
 {
 	struct generic_pm_domain *genpd = &pd->genpd;
 	const char *name = pd->genpd.name;
 	struct dev_power_governor *gov = &simple_qos_governor;
+	int error;
 
 	if (pd->flags & PD_CPU) {
 		/*
@@ -251,7 +252,11 @@ static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
 	rcar_sysc_power_up(&pd->ch);
 
 finalize:
-	pm_genpd_init(genpd, gov, false);
+	error = pm_genpd_init(genpd, gov, false);
+	if (error)
+		pr_err("Failed to init PM domain %s: %d\n", name, error);
+
+	return error;
 }
 
 static const struct of_device_id rcar_sysc_matches[] __initconst = {
@@ -375,6 +380,9 @@ static int __init rcar_sysc_pd_init(void)
 	pr_debug("%pOF: syscier = 0x%08x\n", np, syscier);
 	iowrite32(syscier, base + SYSCIER);
 
+	/*
+	 * First, create all PM domains
+	 */
 	for (i = 0; i < info->num_areas; i++) {
 		const struct rcar_sysc_area *area = &info->areas[i];
 		struct rcar_sysc_pd *pd;
@@ -397,14 +405,29 @@ static int __init rcar_sysc_pd_init(void)
 		pd->ch.isr_bit = area->isr_bit;
 		pd->flags = area->flags;
 
-		rcar_sysc_pd_setup(pd);
-		if (area->parent >= 0)
-			pm_genpd_add_subdomain(domains->domains[area->parent],
-					       &pd->genpd);
+		error = rcar_sysc_pd_setup(pd);
+		if (error)
+			goto out_put;
 
 		domains->domains[area->isr_bit] = &pd->genpd;
 	}
 
+	/*
+	 * Second, link all PM domains to their parents
+	 */
+	for (i = 0; i < info->num_areas; i++) {
+		const struct rcar_sysc_area *area = &info->areas[i];
+
+		if (!area->name || area->parent < 0)
+			continue;
+
+		error = pm_genpd_add_subdomain(domains->domains[area->parent],
+					       domains->domains[area->isr_bit]);
+		if (error)
+			pr_warn("Failed to add PM subdomain %s to parent %u\n",
+				area->name, area->parent);
+	}
+
 	error = of_genpd_add_provider_onecell(np, &domains->onecell_data);
 
 out_put:
-- 
2.11.0

^ permalink raw reply related

* [PATCH v1 2/4] dt-bindings: mailbox: provide imx-mailbox documentation
From: Oleksij Rempel @ 2018-06-13 11:58 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAA+hA=RNxUHro_92YdORw7FOHba=ZSv4q697npCNKwoMG-_HMw@mail.gmail.com>



On 13.06.2018 13:05, Dong Aisheng wrote:
> Hi Oleksij,
> 
> Some more comments besides Rob's:
> 
> On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  .../bindings/mailbox/imx-mailbox.txt          | 35 +++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> new file mode 100644
>> index 000000000000..a45604b33039
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> @@ -0,0 +1,35 @@
>> +i.MX Messaging Unit
>> +===================
>> +
>> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most cases
>> +they are accessible from all Processor Units. On one hand, at least for mailbox functionality,
>> +it makes no difference which application or processor is using which set of the MU. On
>> +other hand, the register sets for each of the MU parts are not identical.
>> +
>> +Required properties:
>> +- compatible : Shell be one of:
>> +                    "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
> 
> What's current requirement to distinguish Side A and B?
> I see current code, side A only does extra clear of xCR register but none for B.
> Is this a generic approach or something customized?

A and B side have more then one BIT difference, and there is no way to
see the difference by software. Current driver do not make use of it,
but in devicetree we describe HW not SW. If HW is different, it should
have different compatibles.

> BTW can we name it more generically?e.g. fsl,imx7s-mu.
> And using a property to indicate whether it's side A or B if really required.
> 
> Furthermore, AFAIK MX7 MU is derived from MX6SX.
> Should we add and use fsl,imx6sx-mu instead?

As soon as some one will test this driver on imx6sx and confirm it
working, i'll add fsl,imx6sx-mu-a/b as well.

> e.g.
>  - compatible : Shell be one of:
>                        "fsl,imx6sx-mu" and "fsl,imx7s-mu".
> 
> Regards
> Dong Aisheng
> 
>> +- reg :        physical base address of the mailbox and length of
>> +               memory mapped region.
>> +- #mbox-cells: Common mailbox binding property to identify the number
>> +               of cells required for the mailbox specifier. Should be 1.
>> +- interrupts : interrupt number. The interrupt specifier format
>> +               depends on the interrupt controller parent.
>> +- clocks     :  phandle to the input clock.
>> +
>> +Example:
>> +       mu0a: mu at 30aa0000 {
>> +               compatible = "fsl,imx7s-mu-a";
>> +               reg = <0x30aa0000 0x28>;
>> +               interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>> +               clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> +               #mbox-cells = <1>;
>> +       };
>> +
>> +       mu0b: mu at 30ab0000 {
>> +               compatible = "fsl,imx7s-mu-b";
>> +               reg = <0x30ab0000 0x28>;
>> +               interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>> +               clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> +               #mbox-cells = <1>;
>> +       };
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180613/a91c9c57/attachment-0001.sig>

^ permalink raw reply

* [PATCH v1 2/4] dt-bindings: mailbox: provide imx-mailbox documentation
From: Oleksij Rempel @ 2018-06-13 11:59 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180612194142.GA16521@rob-hp-laptop>



On 12.06.2018 21:41, Rob Herring wrote:
> On Fri, Jun 01, 2018 at 08:58:19AM +0200, Oleksij Rempel wrote:
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  .../bindings/mailbox/imx-mailbox.txt          | 35 +++++++++++++++++++
>>  1 file changed, 35 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> new file mode 100644
>> index 000000000000..a45604b33039
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt
>> @@ -0,0 +1,35 @@
>> +i.MX Messaging Unit
>> +===================
>> +
>> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most cases
>> +they are accessible from all Processor Units. On one hand, at least for mailbox functionality,
>> +it makes no difference which application or processor is using which set of the MU. On
> 
> Please wrap lines correctly (<80).
> 
>> +other hand, the register sets for each of the MU parts are not identical.
>> +
>> +Required properties:
>> +- compatible :	Shell be one of:
>> +                    "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D
>> +- reg : 	physical base address of the mailbox and length of
> 
> Mix of space and tab.
> 
>> +		memory mapped region.
>> +- #mbox-cells:	Common mailbox binding property to identify the number
>> +		of cells required for the mailbox specifier. Should be 1.
>> +- interrupts :	interrupt number. The interrupt specifier format
>> +		depends on the interrupt controller parent.
> 
> Just need to say how many interrupts and what they are if more than 1.
> 
>> +- clocks     :  phandle to the input clock.
>> +
>> +Example:
>> +	mu0a: mu at 30aa0000 {
> 
> mailbox at ...
> 
>> +		compatible = "fsl,imx7s-mu-a";
>> +		reg = <0x30aa0000 0x28>;
>> +		interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> +		#mbox-cells = <1>;
>> +	};
>> +
>> +	mu0b: mu at 30ab0000 {
> 
> mailbox at ...
> 
>> +		compatible = "fsl,imx7s-mu-b";
>> +		reg = <0x30ab0000 0x28>;
>> +		interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&clks IMX7D_MU_ROOT_CLK>;
>> +		#mbox-cells = <1>;
>> +	};

thx. will fix it.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180613/3d9a7f9d/attachment.sig>

^ permalink raw reply

* [PATCH v7 5/6] regulator: pfuze100-regulator: provide pm_power_off_prepare handler
From: Oleksij Rempel @ 2018-06-13 12:03 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180517055014.6607-6-o.rempel@pengutronix.de>

Hi Mark,

Rob ACKed DT bindings, Rafael ACKed export of pm_power_off_prepare. I
need your OK for regulator patch.

On 17.05.2018 07:50, Oleksij Rempel wrote:
> On some boards the SoC can use one pin "PMIC_STBY_REQ" to notify th PMIC
> about state changes. In this case internal state of PMIC must be
> preconfigured for upcomming state change.
> It works fine with the current regulator framework, except with the
> power-off case.
> 
> This patch is providing an optional pm_power_off_prepare handler
> which will configure standby state of the PMIC to disable all power lines.
> 
> In my power consumption test on RIoTBoard, I got the following results:
> power off without this patch:	320 mA
> power off with this patch:	2   mA
> suspend to ram:			40  mA
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/regulator/pfuze100-regulator.c | 92 ++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
> index 63922a2167e5..f6c276ed91d8 100644
> --- a/drivers/regulator/pfuze100-regulator.c
> +++ b/drivers/regulator/pfuze100-regulator.c
> @@ -28,6 +28,7 @@
>  #include <linux/regulator/pfuze100.h>
>  #include <linux/i2c.h>
>  #include <linux/slab.h>
> +#include <linux/kallsyms.h>
>  #include <linux/regmap.h>
>  
>  #define PFUZE_NUMREGS		128
> @@ -42,11 +43,17 @@
>  
>  #define PFUZE100_COINVOL	0x1a
>  #define PFUZE100_SW1ABVOL	0x20
> +#define PFUZE100_SW1ABMODE	0x23
>  #define PFUZE100_SW1CVOL	0x2e
> +#define PFUZE100_SW1CMODE	0x31
>  #define PFUZE100_SW2VOL		0x35
> +#define PFUZE100_SW2MODE	0x38
>  #define PFUZE100_SW3AVOL	0x3c
> +#define PFUZE100_SW3AMODE	0x3f
>  #define PFUZE100_SW3BVOL	0x43
> +#define PFUZE100_SW3BMODE	0x46
>  #define PFUZE100_SW4VOL		0x4a
> +#define PFUZE100_SW4MODE	0x4d
>  #define PFUZE100_SWBSTCON1	0x66
>  #define PFUZE100_VREFDDRCON	0x6a
>  #define PFUZE100_VSNVSVOL	0x6b
> @@ -57,6 +64,13 @@
>  #define PFUZE100_VGEN5VOL	0x70
>  #define PFUZE100_VGEN6VOL	0x71
>  
> +#define PFUZE100_SWxMODE_MASK	0xf
> +#define PFUZE100_SWxMODE_APS_APS	0x8
> +#define PFUZE100_SWxMODE_APS_OFF	0x4
> +
> +#define PFUZE100_VGENxLPWR	BIT(6)
> +#define PFUZE100_VGENxSTBY	BIT(5)
> +
>  enum chips { PFUZE100, PFUZE200, PFUZE3000 = 3 };
>  
>  struct pfuze_regulator {
> @@ -489,6 +503,69 @@ static inline struct device_node *match_of_node(int index)
>  }
>  #endif
>  
> +static struct pfuze_chip *syspm_pfuze_chip;
> +
> +static void pfuze_power_off_prepare(void)
> +{
> +	dev_info(syspm_pfuze_chip->dev, "Configure standy mode for power off");
> +
> +	/* Switch from default mode: APS/APS to APS/Off */
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW1ABMODE,
> +			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW1CMODE,
> +			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW2MODE,
> +			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW3AMODE,
> +			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW3BMODE,
> +			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_SW4MODE,
> +			   PFUZE100_SWxMODE_MASK, PFUZE100_SWxMODE_APS_OFF);
> +
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN1VOL,
> +			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> +			   PFUZE100_VGENxSTBY);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN2VOL,
> +			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> +			   PFUZE100_VGENxSTBY);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN3VOL,
> +			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> +			   PFUZE100_VGENxSTBY);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN4VOL,
> +			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> +			   PFUZE100_VGENxSTBY);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN5VOL,
> +			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> +			   PFUZE100_VGENxSTBY);
> +	regmap_update_bits(syspm_pfuze_chip->regmap, PFUZE100_VGEN6VOL,
> +			   PFUZE100_VGENxLPWR | PFUZE100_VGENxSTBY,
> +			   PFUZE100_VGENxSTBY);
> +}
> +
> +static int pfuze_power_off_prepare_init(struct pfuze_chip *pfuze_chip)
> +{
> +	if (pfuze_chip->chip_id != PFUZE100) {
> +		dev_warn(pfuze_chip->dev, "Requested pm_power_off_prepare handler for not supported chip\n");
> +		return -ENODEV;
> +	}
> +
> +	if (pm_power_off_prepare) {
> +		dev_warn(pfuze_chip->dev, "pm_power_off_prepare is already registered.\n");
> +		return -EBUSY;
> +	}
> +
> +	if (syspm_pfuze_chip) {
> +		dev_warn(pfuze_chip->dev, "syspm_pfuze_chip is already set.\n");
> +		return -EBUSY;
> +	}
> +
> +	syspm_pfuze_chip = pfuze_chip;
> +	pm_power_off_prepare = pfuze_power_off_prepare;
> +
> +	return 0;
> +}
> +
>  static int pfuze_identify(struct pfuze_chip *pfuze_chip)
>  {
>  	unsigned int value;
> @@ -659,6 +736,20 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
>  		}
>  	}
>  
> +	if (of_property_read_bool(client->dev.of_node,
> +				  "fsl,pmic-stby-poweroff"))
> +		return pfuze_power_off_prepare_init(pfuze_chip);
> +
> +	return 0;
> +}
> +
> +static int pfuze100_regulator_remove(struct i2c_client *client)
> +{
> +	if (syspm_pfuze_chip) {
> +		syspm_pfuze_chip = NULL;
> +		pm_power_off_prepare = NULL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -669,6 +760,7 @@ static struct i2c_driver pfuze_driver = {
>  		.of_match_table = pfuze_dt_ids,
>  	},
>  	.probe = pfuze100_regulator_probe,
> +	.remove = pfuze100_regulator_remove,
>  };
>  module_i2c_driver(pfuze_driver);
>  
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180613/358050e0/attachment.sig>

^ permalink raw reply

* [PATCH] clk: meson-gxbb: set fclk_div2 as CLK_IS_CRITICAL
From: Neil Armstrong @ 2018-06-13 12:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Amlogic Meson GXBB & GXL platforms, the SCPI Cortex-M4 Co-Processor
seems to be dependent on the FCLK_DIV2 to be operationnal.

The issue occured since v4.17-rc1 by freezing the kernel boot when
the 'schedutil' cpufreq governor was selected as default :

  [   12.071837] scpi_protocol scpi: SCP Protocol 0.0 Firmware 0.0.0 version
  domain-0 init dvfs: 4
  [   12.087757] hctosys: unable to open rtc device (rtc0)
  [   12.087907] cfg80211: Loading compiled-in X.509 certificates for regulatory database
  [   12.102241] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'

But when disabling the MMC driver, the boot finished but cpufreq failed to
change the CPU frequency :

  [   12.153045] cpufreq: __target_index: Failed to change cpu frequency: -5

A bisect between v4.16 and v4.16-rc1 gave the 05f814402d61 commit to be
the first bad commit.
This commit added support for the missing clock gates before the fixed PLL
fixed dividers (FCLK_DIVx) and the clock framework basically disabled
all the unused fixed dividers, thus disabled a critical clock path for
the SCPI Co-Processor.

This patch simply sets the FCLK_DIV2 gate as critical to ensure
nobody can disable it.

Fixes: 05f814402d61 ("clk: meson: add fdiv clock gates")
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/gxbb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
index b1e4d95..0e053c1 100644
--- a/drivers/clk/meson/gxbb.c
+++ b/drivers/clk/meson/gxbb.c
@@ -511,6 +511,7 @@ static struct clk_regmap gxbb_fclk_div2 = {
 		.ops = &clk_regmap_gate_ops,
 		.parent_names = (const char *[]){ "fclk_div2_div" },
 		.num_parents = 1,
+		.flags = CLK_IS_CRITICAL,
 	},
 };
 
-- 
2.7.4

^ permalink raw reply related

* [PATCH v1 4/4] mailbox: Add support for i.MX7D messaging unit
From: Dong Aisheng @ 2018-06-13 12:21 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180601065821.28234-5-o.rempel@pengutronix.de>

Hi Oleksij,

On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> The Mailbox controller is able to send messages (up to 4 32 bit words)
> between the endpoints.

Could we really be able to send up to 4 42bit words with this driver?

It looks to me the current Mailbox framework is more designed for share mem
transfer which does not fit i.MX MU well.

>
> This driver was tested using the mailbox-test driver sending messages
> between the Cortex-A7 and the Cortex-M4.

Would you please provide a guide on how to test it quickly?
I may want to give a test.

>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/mailbox/Kconfig       |   6 +
>  drivers/mailbox/Makefile      |   2 +
>  drivers/mailbox/imx-mailbox.c | 289 ++++++++++++++++++++++++++++++++++
>  3 files changed, 297 insertions(+)
>  create mode 100644 drivers/mailbox/imx-mailbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index a2bb27446dce..e1d2738a2e4c 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,12 @@ config ARM_MHU
>           The controller has 3 mailbox channels, the last of which can be
>           used in Secure mode only.
>
> +config IMX_MBOX
> +       tristate "iMX Mailbox"
> +       depends on SOC_IMX7D || COMPILE_TEST
> +       help
> +         Mailbox implementation for iMX7D Messaging Unit (MU).
> +
>  config PLATFORM_MHU
>         tristate "Platform MHU Mailbox"
>         depends on OF
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index cc23c3a43fcd..ba2fe1b6dd62 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
>
>  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
>
> +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> +
>  obj-$(CONFIG_PLATFORM_MHU)     += platform_mhu.o
>
>  obj-$(CONFIG_PL320_MBOX)       += pl320-ipc.o
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> new file mode 100644
> index 000000000000..2bc9f11393b1
> --- /dev/null
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +/* Transmit Register */
> +#define IMX_MU_xTRn(x)         (0x00 + 4 * (x))
> +/* Receive Register */
> +#define IMX_MU_xRRn(x)         (0x10 + 4 * (x))
> +/* Status Register */
> +#define IMX_MU_xSR             0x20
> +#define IMX_MU_xSR_TEn(x)      BIT(20 + (x))
> +#define IMX_MU_xSR_RFn(x)      BIT(24 + (x))
> +#define IMX_MU_xSR_BRDIP       BIT(9)
> +
> +/* Control Register */
> +#define IMX_MU_xCR             0x24
> +/* Transmit Interrupt Enable */
> +#define IMX_MU_xCR_TIEn(x)     BIT(20 + (x))
> +/* Receive Interrupt Enable */
> +#define IMX_MU_xCR_RIEn(x)     BIT(24 + (x))
> +
> +#define IMX_MU_MAX_CHANS       4u
> +
> +struct imx_mu_priv;
> +
> +struct imx_mu_cfg {
> +       unsigned int            chans;
> +       void (*init_hw)(struct imx_mu_priv *priv);
> +};
> +
> +struct imx_mu_con_priv {
> +       int                     irq;
> +       unsigned int            bidx;
> +       unsigned int            idx;
> +};
> +
> +struct imx_mu_priv {
> +       struct device           *dev;
> +       const struct imx_mu_cfg *dcfg;
> +       void __iomem            *base;
> +
> +       struct mbox_controller  mbox;
> +       struct mbox_chan        mbox_chans[IMX_MU_MAX_CHANS];
> +
> +       struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
> +       struct clk              *clk;
> +};
> +
> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
> +{
> +       return container_of(mbox, struct imx_mu_priv, mbox);
> +}
> +
> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
> +{
> +       iowrite32(val, priv->base + offs);
> +}
> +
> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
> +{
> +       return ioread32(priv->base + offs);
> +}
> +
> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr)
> +{
> +       u32 val;
> +
> +       val = imx_mu_read(priv, offs);
> +       val &= ~clr;
> +       val |= set;
> +       imx_mu_write(priv, val, offs);
> +
> +       return val;
> +}
> +
> +static irqreturn_t imx_mu_isr(int irq, void *p)
> +{
> +       struct mbox_chan *chan = p;
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);

Please do in reversed order from long to short

> +
> +       u32 val, dat;
> +
> +       val = imx_mu_read(priv, IMX_MU_xSR);
> +       val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
> +       if (!val)
> +               return IRQ_NONE;
> +
> +       if (val & IMX_MU_xSR_TEn(cp->bidx)) {
> +               imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->bidx));
> +               mbox_chan_txdone(chan, 0);
> +       }
> +
> +       if (val & IMX_MU_xSR_RFn(cp->bidx)) {
> +               dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +               mbox_chan_received_data(chan, (void *)&dat);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static bool imx_mu_last_tx_done(struct mbox_chan *chan)
> +{
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +       u32 val;
> +
> +       val = imx_mu_read(priv, IMX_MU_xSR);
> +       /* test if transmit register is empty */
> +       return (!(val & IMX_MU_xSR_TEn(cp->bidx)));
> +}
> +
> +static int imx_mu_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +       u32 *arg = data;
> +
> +       if (imx_mu_last_tx_done(chan))

return true for tx_done?
Or maybe better imx_mu_is_busy?

> +               return -EBUSY;
> +
> +       imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
> +       imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
> +
> +       return 0;
> +}
> +
> +static int imx_mu_startup(struct mbox_chan *chan)
> +{
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +       int ret;
> +
> +       ret = request_irq(cp->irq, imx_mu_isr,
> +                         IRQF_SHARED, "imx_mu_chan", chan);

This looks me to a bit strange as all virtual channels interrupts
line actually are the same. And we request that same irq line
repeatedly here with the same irq handler.

> +       if (ret) {
> +               dev_err(chan->mbox->dev,
> +                       "Unable to acquire IRQ %d\n", cp->irq);
> +               return ret;
> +       }
> +
> +       imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
> +
> +       return 0;
> +}
> +
> +static void imx_mu_shutdown(struct mbox_chan *chan)
> +{
> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +       struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +       imx_mu_rmw(priv, IMX_MU_xCR, 0,
> +                  IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp->bidx));
> +
> +       free_irq(cp->irq, chan);
> +}
> +
> +static const struct mbox_chan_ops imx_mu_ops = {
> +       .send_data = imx_mu_send_data,
> +       .startup = imx_mu_startup,
> +       .shutdown = imx_mu_shutdown,
> +       .last_tx_done = imx_mu_last_tx_done,

Do we really need this?
Looking at the code, it seems .last_tx_done() is only called for polling mode.
But what you set below is:
priv->mbox.txdone_irq = true;

Or am i missed something?

> +};
> +
> +static int imx_mu_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct resource *iomem;
> +       struct imx_mu_priv *priv;
> +       const struct imx_mu_cfg *dcfg;
> +       unsigned int i, chans;
> +       int irq, ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       dcfg = of_device_get_match_data(dev);
> +       if (!dcfg)
> +               return -EINVAL;
> +
> +       priv->dcfg = dcfg;
> +       priv->dev = dev;
> +
> +       iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       priv->base = devm_ioremap_resource(&pdev->dev, iomem);
> +       if (IS_ERR(priv->base))
> +               return PTR_ERR(priv->base);
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq <= 0)
> +               return irq < 0 ? irq : -EINVAL;

Is it possible == 0?

> +
> +       priv->clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(priv->clk)) {
> +               if (PTR_ERR(priv->clk) == -ENOENT) {
> +                       priv->clk = NULL;
> +               } else {
> +                       dev_err(dev, "Failed to get clock\n");

The line looks not be quite meaningful as it may be defer probe.

> +                       return PTR_ERR(priv->clk);
> +               }
> +       }
> +
> +       ret = clk_prepare_enable(priv->clk);
> +       if (ret) {
> +               dev_err(dev, "Failed to enable clock\n");
> +               return ret;
> +       }
> +
> +       chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
> +       /* Initialize channel identifiers */
> +       for (i = 0; i < chans; i++) {
> +               struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +               cp->bidx = 3 - i;
> +               cp->idx = i;
> +               cp->irq = irq;
> +               priv->mbox_chans[i].con_priv = cp;
> +       }
> +
> +       priv->mbox.dev = dev;
> +       priv->mbox.ops = &imx_mu_ops;
> +       priv->mbox.chans = priv->mbox_chans;
> +       priv->mbox.num_chans = chans;
> +       priv->mbox.txdone_irq = true;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       if (priv->dcfg->init_hw)
> +               priv->dcfg->init_hw(priv);
> +
> +       return mbox_controller_register(&priv->mbox);
> +}
> +
> +static int imx_mu_remove(struct platform_device *pdev)
> +{
> +       struct imx_mu_priv *priv = platform_get_drvdata(pdev);
> +
> +       mbox_controller_unregister(&priv->mbox);
> +       clk_disable_unprepare(priv->clk);
> +
> +       return 0;
> +}
> +
> +
> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv)
> +{
> +       /* Set default config */
> +       imx_mu_write(priv, 0, IMX_MU_xCR);
> +}
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
> +       .chans = IMX_MU_MAX_CHANS,
> +       .init_hw = imx_mu_init_imx7d_a,
> +};
> +
> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
> +       .chans = IMX_MU_MAX_CHANS,
> +};
> +
> +static const struct of_device_id imx_mu_dt_ids[] = {
> +       { .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
> +       { .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> +
> +static struct platform_driver imx_mu_driver = {
> +       .probe          = imx_mu_probe,
> +       .remove         = imx_mu_remove,
> +       .driver = {
> +               .name   = "imx_mu",
> +               .of_match_table = imx_mu_dt_ids,
> +       },
> +};
> +module_platform_driver(imx_mu_driver);
> +
> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> +MODULE_DESCRIPTION("Message Unit driver for i.MX7");

s/i.MX7/i.MX

Regards
Dong Aisheng

> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Matthew Wilcox @ 2018-06-13 12:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2~3rI_9nj8b0455904559eucas1p2C@eucas1p2.samsung.com>

On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.

I think the documentation for this function needs improving.  For example,
GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
At the very least, the kernel-doc needs:

 * Context: Process context (may sleep even if GFP flags indicate otherwise).

Unless someone wants to rework this allocator to use spinlocks instead
of mutexes ...

^ permalink raw reply

* [PATCH v1 4/4] mailbox: Add support for i.MX7D messaging unit
From: Dong Aisheng @ 2018-06-13 12:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAA+hA=RpoRXUK+jB18F4OeYE9PG73SDa_NLYeN5Q2G3B1bC-JA@mail.gmail.com>

Copy linux-imx at nxp.com and more related guys to comment

On Wed, Jun 13, 2018 at 8:21 PM, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Oleksij,
>
> On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>> The Mailbox controller is able to send messages (up to 4 32 bit words)
>> between the endpoints.
>
> Could we really be able to send up to 4 42bit words with this driver?
>
> It looks to me the current Mailbox framework is more designed for share mem
> transfer which does not fit i.MX MU well.
>
>>
>> This driver was tested using the mailbox-test driver sending messages
>> between the Cortex-A7 and the Cortex-M4.
>
> Would you please provide a guide on how to test it quickly?
> I may want to give a test.
>
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>  drivers/mailbox/Kconfig       |   6 +
>>  drivers/mailbox/Makefile      |   2 +
>>  drivers/mailbox/imx-mailbox.c | 289 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 297 insertions(+)
>>  create mode 100644 drivers/mailbox/imx-mailbox.c
>>
>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
>> index a2bb27446dce..e1d2738a2e4c 100644
>> --- a/drivers/mailbox/Kconfig
>> +++ b/drivers/mailbox/Kconfig
>> @@ -15,6 +15,12 @@ config ARM_MHU
>>           The controller has 3 mailbox channels, the last of which can be
>>           used in Secure mode only.
>>
>> +config IMX_MBOX
>> +       tristate "iMX Mailbox"
>> +       depends on SOC_IMX7D || COMPILE_TEST
>> +       help
>> +         Mailbox implementation for iMX7D Messaging Unit (MU).
>> +
>>  config PLATFORM_MHU
>>         tristate "Platform MHU Mailbox"
>>         depends on OF
>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
>> index cc23c3a43fcd..ba2fe1b6dd62 100644
>> --- a/drivers/mailbox/Makefile
>> +++ b/drivers/mailbox/Makefile
>> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST)      += mailbox-test.o
>>
>>  obj-$(CONFIG_ARM_MHU)  += arm_mhu.o
>>
>> +obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>> +
>>  obj-$(CONFIG_PLATFORM_MHU)     += platform_mhu.o
>>
>>  obj-$(CONFIG_PL320_MBOX)       += pl320-ipc.o
>> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
>> new file mode 100644
>> index 000000000000..2bc9f11393b1
>> --- /dev/null
>> +++ b/drivers/mailbox/imx-mailbox.c
>> @@ -0,0 +1,289 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018 Pengutronix, Oleksij Rempel <o.rempel@pengutronix.de>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +/* Transmit Register */
>> +#define IMX_MU_xTRn(x)         (0x00 + 4 * (x))
>> +/* Receive Register */
>> +#define IMX_MU_xRRn(x)         (0x10 + 4 * (x))
>> +/* Status Register */
>> +#define IMX_MU_xSR             0x20
>> +#define IMX_MU_xSR_TEn(x)      BIT(20 + (x))
>> +#define IMX_MU_xSR_RFn(x)      BIT(24 + (x))
>> +#define IMX_MU_xSR_BRDIP       BIT(9)
>> +
>> +/* Control Register */
>> +#define IMX_MU_xCR             0x24
>> +/* Transmit Interrupt Enable */
>> +#define IMX_MU_xCR_TIEn(x)     BIT(20 + (x))
>> +/* Receive Interrupt Enable */
>> +#define IMX_MU_xCR_RIEn(x)     BIT(24 + (x))
>> +
>> +#define IMX_MU_MAX_CHANS       4u
>> +
>> +struct imx_mu_priv;
>> +
>> +struct imx_mu_cfg {
>> +       unsigned int            chans;
>> +       void (*init_hw)(struct imx_mu_priv *priv);
>> +};
>> +
>> +struct imx_mu_con_priv {
>> +       int                     irq;
>> +       unsigned int            bidx;
>> +       unsigned int            idx;
>> +};
>> +
>> +struct imx_mu_priv {
>> +       struct device           *dev;
>> +       const struct imx_mu_cfg *dcfg;
>> +       void __iomem            *base;
>> +
>> +       struct mbox_controller  mbox;
>> +       struct mbox_chan        mbox_chans[IMX_MU_MAX_CHANS];
>> +
>> +       struct imx_mu_con_priv  con_priv[IMX_MU_MAX_CHANS];
>> +       struct clk              *clk;
>> +};
>> +
>> +static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
>> +{
>> +       return container_of(mbox, struct imx_mu_priv, mbox);
>> +}
>> +
>> +static void imx_mu_write(struct imx_mu_priv *priv, u32 val, u32 offs)
>> +{
>> +       iowrite32(val, priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_read(struct imx_mu_priv *priv, u32 offs)
>> +{
>> +       return ioread32(priv->base + offs);
>> +}
>> +
>> +static u32 imx_mu_rmw(struct imx_mu_priv *priv, u32 offs, u32 set, u32 clr)
>> +{
>> +       u32 val;
>> +
>> +       val = imx_mu_read(priv, offs);
>> +       val &= ~clr;
>> +       val |= set;
>> +       imx_mu_write(priv, val, offs);
>> +
>> +       return val;
>> +}
>> +
>> +static irqreturn_t imx_mu_isr(int irq, void *p)
>> +{
>> +       struct mbox_chan *chan = p;
>> +       struct imx_mu_con_priv *cp = chan->con_priv;
>> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>
> Please do in reversed order from long to short
>
>> +
>> +       u32 val, dat;
>> +
>> +       val = imx_mu_read(priv, IMX_MU_xSR);
>> +       val &= IMX_MU_xSR_TEn(cp->bidx) | IMX_MU_xSR_RFn(cp->bidx);
>> +       if (!val)
>> +               return IRQ_NONE;
>> +
>> +       if (val & IMX_MU_xSR_TEn(cp->bidx)) {
>> +               imx_mu_rmw(priv, IMX_MU_xCR, 0, IMX_MU_xCR_TIEn(cp->bidx));
>> +               mbox_chan_txdone(chan, 0);
>> +       }
>> +
>> +       if (val & IMX_MU_xSR_RFn(cp->bidx)) {
>> +               dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>> +               mbox_chan_received_data(chan, (void *)&dat);
>> +       }
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static bool imx_mu_last_tx_done(struct mbox_chan *chan)
>> +{
>> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +       struct imx_mu_con_priv *cp = chan->con_priv;
>> +       u32 val;
>> +
>> +       val = imx_mu_read(priv, IMX_MU_xSR);
>> +       /* test if transmit register is empty */
>> +       return (!(val & IMX_MU_xSR_TEn(cp->bidx)));
>> +}
>> +
>> +static int imx_mu_send_data(struct mbox_chan *chan, void *data)
>> +{
>> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +       struct imx_mu_con_priv *cp = chan->con_priv;
>> +       u32 *arg = data;
>> +
>> +       if (imx_mu_last_tx_done(chan))
>
> return true for tx_done?
> Or maybe better imx_mu_is_busy?
>
>> +               return -EBUSY;
>> +
>> +       imx_mu_write(priv, *arg, IMX_MU_xTRn(cp->idx));
>> +       imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xSR_TEn(cp->bidx), 0);
>> +
>> +       return 0;
>> +}
>> +
>> +static int imx_mu_startup(struct mbox_chan *chan)
>> +{
>> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +       struct imx_mu_con_priv *cp = chan->con_priv;
>> +       int ret;
>> +
>> +       ret = request_irq(cp->irq, imx_mu_isr,
>> +                         IRQF_SHARED, "imx_mu_chan", chan);
>
> This looks me to a bit strange as all virtual channels interrupts
> line actually are the same. And we request that same irq line
> repeatedly here with the same irq handler.
>
>> +       if (ret) {
>> +               dev_err(chan->mbox->dev,
>> +                       "Unable to acquire IRQ %d\n", cp->irq);
>> +               return ret;
>> +       }
>> +
>> +       imx_mu_rmw(priv, IMX_MU_xCR, IMX_MU_xCR_RIEn(cp->bidx), 0);
>> +
>> +       return 0;
>> +}
>> +
>> +static void imx_mu_shutdown(struct mbox_chan *chan)
>> +{
>> +       struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +       struct imx_mu_con_priv *cp = chan->con_priv;
>> +
>> +       imx_mu_rmw(priv, IMX_MU_xCR, 0,
>> +                  IMX_MU_xCR_TIEn(cp->bidx) | IMX_MU_xCR_RIEn(cp->bidx));
>> +
>> +       free_irq(cp->irq, chan);
>> +}
>> +
>> +static const struct mbox_chan_ops imx_mu_ops = {
>> +       .send_data = imx_mu_send_data,
>> +       .startup = imx_mu_startup,
>> +       .shutdown = imx_mu_shutdown,
>> +       .last_tx_done = imx_mu_last_tx_done,
>
> Do we really need this?
> Looking at the code, it seems .last_tx_done() is only called for polling mode.
> But what you set below is:
> priv->mbox.txdone_irq = true;
>
> Or am i missed something?
>
>> +};
>> +
>> +static int imx_mu_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct resource *iomem;
>> +       struct imx_mu_priv *priv;
>> +       const struct imx_mu_cfg *dcfg;
>> +       unsigned int i, chans;
>> +       int irq, ret;
>> +
>> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       dcfg = of_device_get_match_data(dev);
>> +       if (!dcfg)
>> +               return -EINVAL;
>> +
>> +       priv->dcfg = dcfg;
>> +       priv->dev = dev;
>> +
>> +       iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       priv->base = devm_ioremap_resource(&pdev->dev, iomem);
>> +       if (IS_ERR(priv->base))
>> +               return PTR_ERR(priv->base);
>> +
>> +       irq = platform_get_irq(pdev, 0);
>> +       if (irq <= 0)
>> +               return irq < 0 ? irq : -EINVAL;
>
> Is it possible == 0?
>
>> +
>> +       priv->clk = devm_clk_get(dev, NULL);
>> +       if (IS_ERR(priv->clk)) {
>> +               if (PTR_ERR(priv->clk) == -ENOENT) {
>> +                       priv->clk = NULL;
>> +               } else {
>> +                       dev_err(dev, "Failed to get clock\n");
>
> The line looks not be quite meaningful as it may be defer probe.
>
>> +                       return PTR_ERR(priv->clk);
>> +               }
>> +       }
>> +
>> +       ret = clk_prepare_enable(priv->clk);
>> +       if (ret) {
>> +               dev_err(dev, "Failed to enable clock\n");
>> +               return ret;
>> +       }
>> +
>> +       chans = min(dcfg->chans, IMX_MU_MAX_CHANS);
>> +       /* Initialize channel identifiers */
>> +       for (i = 0; i < chans; i++) {
>> +               struct imx_mu_con_priv *cp = &priv->con_priv[i];
>> +
>> +               cp->bidx = 3 - i;
>> +               cp->idx = i;
>> +               cp->irq = irq;
>> +               priv->mbox_chans[i].con_priv = cp;
>> +       }
>> +
>> +       priv->mbox.dev = dev;
>> +       priv->mbox.ops = &imx_mu_ops;
>> +       priv->mbox.chans = priv->mbox_chans;
>> +       priv->mbox.num_chans = chans;
>> +       priv->mbox.txdone_irq = true;
>> +
>> +       platform_set_drvdata(pdev, priv);
>> +
>> +       if (priv->dcfg->init_hw)
>> +               priv->dcfg->init_hw(priv);
>> +
>> +       return mbox_controller_register(&priv->mbox);
>> +}
>> +
>> +static int imx_mu_remove(struct platform_device *pdev)
>> +{
>> +       struct imx_mu_priv *priv = platform_get_drvdata(pdev);
>> +
>> +       mbox_controller_unregister(&priv->mbox);
>> +       clk_disable_unprepare(priv->clk);
>> +
>> +       return 0;
>> +}
>> +
>> +
>> +static void imx_mu_init_imx7d_a(struct imx_mu_priv *priv)
>> +{
>> +       /* Set default config */
>> +       imx_mu_write(priv, 0, IMX_MU_xCR);
>> +}
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_a = {
>> +       .chans = IMX_MU_MAX_CHANS,
>> +       .init_hw = imx_mu_init_imx7d_a,
>> +};
>> +
>> +static const struct imx_mu_cfg imx_mu_cfg_imx7d_b = {
>> +       .chans = IMX_MU_MAX_CHANS,
>> +};
>> +
>> +static const struct of_device_id imx_mu_dt_ids[] = {
>> +       { .compatible = "fsl,imx7s-mu-a", .data = &imx_mu_cfg_imx7d_a },
>> +       { .compatible = "fsl,imx7s-mu-b", .data = &imx_mu_cfg_imx7d_b },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
>> +
>> +static struct platform_driver imx_mu_driver = {
>> +       .probe          = imx_mu_probe,
>> +       .remove         = imx_mu_remove,
>> +       .driver = {
>> +               .name   = "imx_mu",
>> +               .of_match_table = imx_mu_dt_ids,
>> +       },
>> +};
>> +module_platform_driver(imx_mu_driver);
>> +
>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>> +MODULE_DESCRIPTION("Message Unit driver for i.MX7");
>
> s/i.MX7/i.MX
>
> Regards
> Dong Aisheng
>
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.17.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH] clk: meson-gxbb: set fclk_div2 as CLK_IS_CRITICAL
From: Jerome Brunet @ 2018-06-13 12:26 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1528892421-12180-1-git-send-email-narmstrong@baylibre.com>

On Wed, 2018-06-13 at 14:20 +0200, Neil Armstrong wrote:
> On Amlogic Meson GXBB & GXL platforms, the SCPI Cortex-M4 Co-Processor
> seems to be dependent on the FCLK_DIV2 to be operationnal.
> 
> The issue occured since v4.17-rc1 by freezing the kernel boot when
> the 'schedutil' cpufreq governor was selected as default :
> 
>   [   12.071837] scpi_protocol scpi: SCP Protocol 0.0 Firmware 0.0.0 version
>   domain-0 init dvfs: 4
>   [   12.087757] hctosys: unable to open rtc device (rtc0)
>   [   12.087907] cfg80211: Loading compiled-in X.509 certificates for regulatory database
>   [   12.102241] cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
> 
> But when disabling the MMC driver, the boot finished but cpufreq failed to
> change the CPU frequency :
> 
>   [   12.153045] cpufreq: __target_index: Failed to change cpu frequency: -5
> 
> A bisect between v4.16 and v4.16-rc1 gave the 05f814402d61 commit to be
> the first bad commit.
> This commit added support for the missing clock gates before the fixed PLL
> fixed dividers (FCLK_DIVx) and the clock framework basically disabled
> all the unused fixed dividers, thus disabled a critical clock path for
> the SCPI Co-Processor.
> 
> This patch simply sets the FCLK_DIV2 gate as critical to ensure
> nobody can disable it.
> 
> Fixes: 05f814402d61 ("clk: meson: add fdiv clock gates")
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>

Good catch !
We'll probably have to check the axg family as well

> ---
>  drivers/clk/meson/gxbb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/meson/gxbb.c b/drivers/clk/meson/gxbb.c
> index b1e4d95..0e053c1 100644
> --- a/drivers/clk/meson/gxbb.c
> +++ b/drivers/clk/meson/gxbb.c
> @@ -511,6 +511,7 @@ static struct clk_regmap gxbb_fclk_div2 = {
>  		.ops = &clk_regmap_gate_ops,
>  		.parent_names = (const char *[]){ "fclk_div2_div" },
>  		.num_parents = 1,
> +		.flags = CLK_IS_CRITICAL,
>  	},
>  };
>  

^ permalink raw reply

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Marek Szyprowski @ 2018-06-13 12:40 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180613122359.GA8695@bombadil.infradead.org>

Hi Matthew,

On 2018-06-13 14:24, Matthew Wilcox wrote:
> On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
>> cma_alloc() function has gfp mask parameter, so users expect that it
>> honors typical memory allocation related flags. The most imporant from
>> the security point of view is handling of __GFP_ZERO flag, because memory
>> allocated by this function usually can be directly remapped to userspace
>> by device drivers as a part of multimedia processing and ignoring this
>> flag might lead to leaking some kernel structures to userspace.
>> Some callers of this function (for example arm64 dma-iommu glue code)
>> already assumed that the allocated buffers are cleared when this flag
>> is set. To avoid such issues, add simple code for clearing newly
>> allocated buffer when __GFP_ZERO flag is set. Callers will be then
>> updated to skip implicit clearing or adjust passed gfp flags.
> I think the documentation for this function needs improving.  For example,
> GFP_ATOMIC does not work (it takes a mutex lock, so it can sleep).
> At the very least, the kernel-doc needs:
>
>   * Context: Process context (may sleep even if GFP flags indicate otherwise).
>
> Unless someone wants to rework this allocator to use spinlocks instead
> of mutexes ...

It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
by the
memory compaction code, which is used in alloc_contig_range(). Right, this
should be also noted in the documentation.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

^ permalink raw reply

* [PATCH v1 4/4] mailbox: Add support for i.MX7D messaging unit
From: Sascha Hauer @ 2018-06-13 12:48 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAA+hA=RpoRXUK+jB18F4OeYE9PG73SDa_NLYeN5Q2G3B1bC-JA@mail.gmail.com>

On Wed, Jun 13, 2018 at 08:21:10PM +0800, Dong Aisheng wrote:
> Hi Oleksij,
> 
> On Fri, Jun 1, 2018 at 2:58 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > The Mailbox controller is able to send messages (up to 4 32 bit words)
> > between the endpoints.
> 
> Could we really be able to send up to 4 42bit words with this driver?
> 
> It looks to me the current Mailbox framework is more designed for share mem
> transfer which does not fit i.MX MU well.

The mailbox framework just defines channels and messages. A message is a
void * which may contain arbitrary data or even no data at all; some
drivers simply ignore the message pointer, so in fact they act as a
doorbell unit only.

There's nothing about shared memory in the mailbox framework, but of
course you can combine a mailbox driver and shared memory to a remote
message mechanism. That could be done with the i.MX MU aswell and would
indeed be a good match for the hardware.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply

* [RFC PATCH 6/8] dts: coresight: Clean up the device tree graph bindings
From: Matt Sealey @ 2018-06-13 12:49 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <a1c0749b-9c97-b45f-716f-a62ae07af278@arm.com>

Suzuki,

Why not use ?unit??

I believe we had this discussion years ago about numbering serial ports and sdhci (i.e. how do you know it?s UART0 or UART1 from just the address? Some SoC?s don?t address sequentially *or* in a forward direction) - I believe it?s not exactly codified in ePAPR, not am I sure where it may be otherwise, but it exists.

I agree with Rob on the slave-mode nonsense, this is an SPI controller concept weirdly stuffed into a directed graph which implicitly tells you the data direction - it?s a rooted tree (just like DT!).

For the case of a funnel each device supplying trace should end up into an input node - numbered with a unit - and all those nodes should point to the output node as endpoints. Describing the hardware as a black box is probably less of a good idea than showing that it?s a funnel, or replicator by showing the internal paths. You wouldn?t need to ?number? ports with a unit except where the HW needs to differentiate between them, and you don?t need reg or a node address to do it.

If you really need to parse full graphs in both directions (find root, find leaf) then could we simply introduce properties which list the phandles of all uplink sources, as linked lists point to the list head?

This gives a way to validate that the graph starts and ends the way we expect, and also allows every port to be associated with being a required path between any two devices without parsing the *whole* graph (although you still need to do that to find the route to sinks).

Ta,
Matt

Sent from my iPhone

> On Jun 13, 2018, at 04:45, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>
> Hi Rob,
>
>> On 12/06/18 21:48, Rob Herring wrote:
>>> On Fri, Jun 01, 2018 at 02:16:05PM +0100, Suzuki K Poulose wrote:
>>> The coresight drivers relied on default bindings for graph
>>> in DT, while reusing the "reg" field of the "ports" to indicate
>>> the actual hardware port number for the connections. However,
>>> with the rules getting stricter w.r.t to the address mismatch
>>> with the label, it is no longer possible to use the port address
>>> field for the hardware port number. Hence, we add an explicit
>>> property to denote the hardware port number, "coresight,hwid"
>>> which must be specified for each "endpoint".
>>>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Sudeep Holla <sudeep.holla@arm.com>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> ---
>>>  .../devicetree/bindings/arm/coresight.txt          | 26 +++++++++---
>>>  drivers/hwtracing/coresight/of_coresight.c         | 46 ++++++++++++++++------
>>>  2 files changed, 54 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
>>> index bd36e40..385581a 100644
>>> --- a/Documentation/devicetree/bindings/arm/coresight.txt
>>> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
>>> @@ -104,7 +104,11 @@ properties to uniquely identify the connection details.
>>>      "slave-mode"
>>>     * Hardware Port number at the component:
>>> -     -  The hardware port number is assumed to be the address of the "port" component.
>>> +   - (Obsolete) The hardware port number is assumed to be the address of the "port" component.
>>> +   - Each "endpoint" must define the hardware port of the local end of the
>>> +     connection using the following property:
>>> +    "coresight,hwid" - 32bit integer, hardware port number at the local end.
>> "coresight" is not a vendor and properties are in the form
>> [<vendor>,]<prop-name>.
>
> OK. The issue here is that a coresight component could be an Arm IP or
> a custom partner IP. So, the vendor could be either arm or the partner id.
> However, this property is kind of a generic one for the Coresight family,
> which is why we opted for "coresight". What is the guideline for such
> cases ?
>
> Or in other words I see the following possible options :
>
> 1) coresight,hwid    - coresight generic
> 2) arm,coresight-hwid    - arm vendor, however the device could be from any vendor.
> 3) hwid            - Generic
> 4) none of the above, something completely different.
>
> What do you recommend from the above ?
>
>>> +
>>>      Example:
>>> @@ -120,6 +124,7 @@ Example:
>>>              etb_in_port: endpoint at 0 {
>> There shouldn't be a unit address here because there is no reg property.
>>>                  slave-mode;
>>>                  remote-endpoint = <&replicator_out_port0>;
>>> +                coresight,hwid = <0>;
>> It doesn't make sense for these to be in the endpoint. If you had
>> multiple endpoints, then you would have to duplicate it. "ports" are
>> a single data stream. "endpoints" are connections to that stream. So if
>> you have a muxed (input) or fanout/1-to-many (output) connection, then
>> you have multiple endpoints.
>
> We do have many-to-1 input (e.g, funnels) and 1-to-many outputs
> (e.g replicators). However, we have (so far) used only one endpoint per
> port.
>
> Also we could potentially have multiple data streams flowing through
> the ports, which gets filtered to different ports in 1-to-many components
> (read programmable-replicator).
>
> So the point is we have a shared path which carries different data
> streams with mux/demux components. I am open for suggestions based on
> the above facts.
>
>> The same applied to the slave-mode property, but that ship has sailed.
>> No reason to continue that though.
>>>              };
>>>          };
>>>      };
>>> @@ -134,6 +139,7 @@ Example:
>>>              tpiu_in_port: endpoint at 0 {
>>>                  slave-mode;
>>>                  remote-endpoint = <&replicator_out_port1>;
>>> +                coresight,hwid = <0>;
>>>              };
>>>          };
>>>      };
>>> @@ -154,6 +160,7 @@ Example:
>>>                  reg = <0>;
>>>                  replicator_out_port0: endpoint {
>>>                      remote-endpoint = <&etb_in_port>;
>>> +                    coresight,hwid = <0>;
>>>                  };
>>>              };
>>>  @@ -161,15 +168,17 @@ Example:
>>>                  reg = <1>;
>>>                  replicator_out_port1: endpoint {
>>>                      remote-endpoint = <&tpiu_in_port>;
>>> +                    coresight,hwid = <1>;
>>>                  };
>>>              };
>>>                /* replicator input port */
>>>              port at 2 {
>>> -                reg = <0>;
>>> +                reg = <1>;
>> This will still get flagged as an error. reg must be 2 here.
>
> Sorry, thats a mistake. I will fix it.
>
> Cheers
> Suzuki
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.

^ permalink raw reply

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Christoph Hellwig @ 2018-06-13 12:52 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180613085851eucas1p20337d050face8ff8ea87674e16a9ccd2~3rI_9nj8b0455904559eucas1p2C@eucas1p2.samsung.com>

On Wed, Jun 13, 2018 at 10:58:37AM +0200, Marek Szyprowski wrote:
> cma_alloc() function has gfp mask parameter, so users expect that it
> honors typical memory allocation related flags. The most imporant from
> the security point of view is handling of __GFP_ZERO flag, because memory
> allocated by this function usually can be directly remapped to userspace
> by device drivers as a part of multimedia processing and ignoring this
> flag might lead to leaking some kernel structures to userspace.
> Some callers of this function (for example arm64 dma-iommu glue code)
> already assumed that the allocated buffers are cleared when this flag
> is set. To avoid such issues, add simple code for clearing newly
> allocated buffer when __GFP_ZERO flag is set. Callers will be then
> updated to skip implicit clearing or adjust passed gfp flags.

dma mapping implementations need to zero all memory returned anyway
(even if a few implementation don't do that yet).

I'd rather keep the zeroing in the common callers.

^ permalink raw reply

* [PATCH] mm: cma: honor __GFP_ZERO flag in cma_alloc()
From: Christoph Hellwig @ 2018-06-13 12:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20180613124001eucas1p2422f7916367ce19fecd40d6131990383~3uKFrT3ML1977219772eucas1p2G@eucas1p2.samsung.com>

On Wed, Jun 13, 2018 at 02:40:00PM +0200, Marek Szyprowski wrote:
> It is not only the matter of the spinlocks. GFP_ATOMIC is not supported 
> by the
> memory compaction code, which is used in alloc_contig_range(). Right, this
> should be also noted in the documentation.

Documentation is good, asserts are better.  The code should reject any
flag not explicitly supported, or even better have its own flags type
with the few actually supported flags.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox