* Re: Extreme time jitter with suspend/resume cycles
From: Thomas Gleixner @ 2017-10-05 11:01 UTC (permalink / raw)
To: Gabriel Beddingfield
Cc: LKML, Stephen Boyd, John Stultz, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Guy Erb, hharte
In-Reply-To: <CAOdF7ntda5TuCDBUTsuPZ0TCofo-GVfBmrGVyPk-hqGgHEkBQw@mail.gmail.com>
On Wed, 4 Oct 2017, Gabriel Beddingfield wrote:
> On Wed, Oct 4, 2017 at 11:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Long story short: you can't always have your low-power clock be your
> monotonic/sched clock.
sched_clock and the clocksource for timekeeping, which feeds monotonic are
not required to be the same thing.
> The SoC we use backs the monotonic clock (sched_clock_register()) with a
Again. monotonic clock != sched clock. The clocksource which feeds the
monotonic timekeeper clock is registered via clocksource_register() & al.
> counter that is high frequency (>10 MHz) in their reference
> implementation. But it does not count when the system is in low-power
> mode. However, it can be configured to use a 32kHz clock that *does*
> count when the system is in low-power mode. So, we started by using this
> clock and setting the CLOCK_SOURCE_SUSPEND_NONSTOP flag. It worked
> great... at first.
>
> Then we found that devices would randomly experience a 36-hour time jump.
> While we don't have a definitive root cause, the current theory is that
> we are getting non-atomic reads because the clock source isn't
> synchronized with the the high frequency clock (which is used for most of
> the digital logic on the SoC).
Groan. Engineering based on theories is doomed to begin with.
Your 36 hour time jump is probably exactly 36.4089 hours as that's
((1 << 32) / 32768) / 3600
i.e. the 32bit rollover of the clocksource. So, if the clocksource->read()
function returns a full 64bit counter value, then it must have protection
against observing the rollover independent of the clock which feeds that
counter. Of course the frequency changes the probablity of observing it,
but still the read function must be protected against observing the
rollover unconditionally.
Which SoC/clocksource driver are you talking about?
> Therefore, we moved the monotonic/sched clock back to the high-frequency source.
Please stop confusing timekeeping clock source and sched clock. They might
be the same physical device but conceptually they are different.
> Meanwhile, we can directly read the RTC clock on this system, and it will
> give us 32kHz resolution and also runs non-stop. Since reads are
> non-atomic, we have to read the registers in a loop. We used this
> register to implement read_persistent_clock64(). Because we have to read
> the registers in a loop, it seemed unfit for use as the monotonic/sched
> clock.
I can understand that. Though, using that value for injecting accurate
sleep time should just work with the existing code no matter how long the
actual sleep time was. The timekeeping core takes the nsec part of the
timespec value retrieved via read_persistent_clock64() into account.
I still have a hard time to figure out what you are trying to achieve.
Thanks,
tglx
^ permalink raw reply
* Re: Extreme time jitter with suspend/resume cycles
From: John Stultz @ 2017-10-05 0:20 UTC (permalink / raw)
To: Gabriel Beddingfield
Cc: Thomas Gleixner, LKML, Stephen Boyd, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Guy Erb, hharte
In-Reply-To: <CAOdF7ntda5TuCDBUTsuPZ0TCofo-GVfBmrGVyPk-hqGgHEkBQw@mail.gmail.com>
On Wed, Oct 4, 2017 at 4:10 PM, Gabriel Beddingfield <gabe@nestlabs.com> wrote:
> Hi Thomas,
>
> Thanks for your reply!
>
> On Wed, Oct 4, 2017 at 11:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> Calling things a hack which might have been thought out carefully does not
>> make people more receptive.
>
> My bad... sorry! You're right. The code in question is better than a "hack."
>
>>> Many ARM systems provide a "persistent clock." Most of them are backed
>>> by a 32kHz clock that gives good precision and makes the delta_delta
>>> hack unnecessary. However, devices that only have single-second
>>> precision for the persistent clock and/or are forced to use the RTC
>>> (whose API only allows for single-second precision) -- they still need
>>> this hack.
>>
>> I have no idea what you are trying to tell me. We know that there are
>> systems which have a clocksource which continues to tick across
>> suspend/resume.
>
> I'm referring to read_persistent_clock64() API... which is distinct from the
> CLOCK_SOURCE_SUSPEND_NONSTOP flag.
>
>> Such clocksources can be flagged with CLOCK_SOURCE_SUSPEND_NONSTOP and the
>> timekeeping resume code uses them when available instead of using the
>> RTC. There are a few of them flagged as such in the kernel, but it might
> ...
>> It's neither a problem of the timekeeping core code to select the
>> appropriate clocksource for a system. That's up to the developer who
>> implemented the SoC/CPU support and made a decision about rating the
>> clocksource(s), which might end up selecting one which stops in suspend.
>
> This was our first approach. However, because of some hardware
> limitations we couldn't
> move the system's monotonic clock to the persistent clock. They had to
> be two different
> clocks. (Details below.)
>
>> Without looking at what it does, I can tell you that making it a config
>> option is not going to fly. It has to be runtime discoverable as we have to
>> support multi platform kernels.
>
> OK. Here's a couple ideas...
>
> APPROACH ONE: Use a heuristic
>
> If read_persistent_clock64() ever returns fractional seconds (as
> apposed to whole seconds), then
> permanently disable the compensation.
This seems reasonable to me as well.
>> Though I still want to know exactly why you think that you need some extra
>> magic in the timekeeping core code. If your system has a clocksource which
>> is not stopping on suspend and it lacks the flag, then this is a one liner
>> patch. If there is something else, then please elaborate.
>
> Long story short: you can't always have your low-power clock be your
> monotonic/sched
> clock.
>
> The SoC we use backs the monotonic clock (sched_clock_register()) with
> a counter that is
> high frequency (>10 MHz) in their reference implementation. But it
> does not count when the
> system is in low-power mode. However, it can be configured to use a
> 32kHz clock that *does*
> count when the system is in low-power mode. So, we started by using
> this clock and setting the
> CLOCK_SOURCE_SUSPEND_NONSTOP flag. It worked great... at first.
>
> Then we found that devices would randomly experience a 36-hour time jump.
> While we don't have a definitive root cause, the current theory is
> that we are getting
> non-atomic reads because the clock source isn't synchronized with the
> the high frequency
> clock (which is used for most of the digital logic on the SoC).
>
> Therefore, we moved the monotonic/sched clock back to the high-frequency source.
>
> Meanwhile, we can directly read the RTC clock on this system, and it
> will give us 32kHz
> resolution and also runs non-stop. Since reads are non-atomic, we have
> to read the
> registers in a loop. We used this register to implement
> read_persistent_clock64().
> Because we have to read the registers in a loop, it seemed unfit for use as the
> monotonic/sched clock.
Yea. I thought arm devices often had read_persistent_clock64() backed
by the 32k timer (which is poor for time initialization but works well
for suspend timing).
Maybe I misunderstood on the first read. Is it then that the
relatively fine-grained read_persistent_clock64() is colliding with
the delta_delta logic that assumes we get coarse 1sec resolution? In
that case the huristic above seems sane.
thanks
-john
^ permalink raw reply
* Re: Extreme time jitter with suspend/resume cycles
From: John Stultz @ 2017-10-05 0:16 UTC (permalink / raw)
To: Gabriel Beddingfield
Cc: LKML, Stephen Boyd, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Guy Erb, hharte, Miroslav Lichvar
In-Reply-To: <CAOdF7nvHfh_M3UQDYjbofNOm0-S_-LXHdmsMY7DeEwgOQCmfFA@mail.gmail.com>
On Wed, Oct 4, 2017 at 9:11 AM, Gabriel Beddingfield <gabe@nestlabs.com> wrote:
> TL;DR: the "delta_delta" hack[1 and 2] in kernel/time/timekeeping.c
> and drivers/rtc/class.c undermines the NTP system. It's not
> appropriate to use if sub-second precision is available. I've attached
> a patch to resolve this... please let me know the ways you hate it.
> :-)
>
> Hello Kernel Timekeeping Maintainers,
>
> We have been developing a device that has very a very aggressive power
> policy, doing suspend/resume cycles a few times a minute ("echo mem >
> /sys/power/state"). In doing so, we found that the system time
> experiences a lot of jitter (compared to, say, an NTP server). It was
> not uncommon for us to see time corrections of 1s to 4s on a regular
> basis. This didn't happen when the device stayed awake, only when it
> was allowed to do suspend/resume.
>
> We found that the problem is an interaction between the NTP code and
> what I call the "delta_delta hack." (see [1] and [2]) This code
> allocates a static variable in a function that contains an offset from
> the system time to the persistent/rtc clock. It uses that time to
> fudge the suspend timestamp so that on resume the sleep time will be
> compensated. It's kind of a statistical hack that assumes things will
> average out. It seems to have two main assumptions:
>
> 1. The persistent/rtc clock has only single-second precision
> 2. The system does not frequently suspend/resume.
> 3. If delta_delta is less than 2 seconds, these assumptions are "true"
>
> Because the delta_delta hack is trying to maintain an offset from the
> system time to the persistent/rtc clock, any minor NTP corrections
> that have occurred since the last suspend will be discarded. However,
> the NTP subsystem isn't notified that this is happening -- and so it
> causes some level of instability in its PLL logic.
So, on resume when we call __timekeeping_inject_sleeptime(), that uses
the TK_CLEAR_NTP which clears the NTP state (sets STA_UNSYNC, etc) .
I'm not sure how else we can notify userspace. It may be that ntpd
doesn't expect the kernel to set things as unsynced and doesn't
recover well, but the proper fix for that probably is in userspace.
>
> This problem affects any device that does "frequent" suspend/resume
> cycles. I.e. any battery-powered "linux" device (like Android phones).
Ironically, if I recall correctly, the delta_delta "hack" originally
came from Android developers who were trying to find a solution to the
~1sec per suspend time error that we had before.
> Many ARM systems provide a "persistent clock." Most of them are backed
> by a 32kHz clock that gives good precision and makes the delta_delta
> hack unnecessary. However, devices that only have single-second
> precision for the persistent clock and/or are forced to use the RTC
> (whose API only allows for single-second precision) -- they still need
> this hack.
>
> I've attached a patch that we developed in-house. I have a feeling you
> won't like it... since it pushes the responsibility on whoever
> configures the kernel. It also ignores the RTC problem (which will
> still affect a lot of battery-powered devices).
>
> Please let me know what you think -- and what the right approach for
> solving this would be.
So I suspect the best solution for you here is: provide some
infrastructure so clocksources that set CLOCK_SOURCE_SUSPEND_NONSTOP
which are not the current clocksource can be used for suspend timing.
We should also figure out how to best handle ntpd in userspace dealing
with frequent suspend/resume cycles. This is problematic, as the
closest analogy is trying driving on the road while frequently falling
asleep. This is not something I think ntpd handles well. I suspect
our options are that any ntp adjustments being made might be made for
far too long (causing potentially massive over-correction) or not at
all, and not at all seems slightly better in my mind.
Miroslav may have other thoughts?
thanks
-john
^ permalink raw reply
* Re: Extreme time jitter with suspend/resume cycles
From: Gabriel Beddingfield @ 2017-10-04 23:10 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Stephen Boyd, John Stultz, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Guy Erb, hharte
In-Reply-To: <alpine.DEB.2.20.1710041931560.2406@nanos>
Hi Thomas,
Thanks for your reply!
On Wed, Oct 4, 2017 at 11:22 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Calling things a hack which might have been thought out carefully does not
> make people more receptive.
My bad... sorry! You're right. The code in question is better than a "hack."
>> Many ARM systems provide a "persistent clock." Most of them are backed
>> by a 32kHz clock that gives good precision and makes the delta_delta
>> hack unnecessary. However, devices that only have single-second
>> precision for the persistent clock and/or are forced to use the RTC
>> (whose API only allows for single-second precision) -- they still need
>> this hack.
>
> I have no idea what you are trying to tell me. We know that there are
> systems which have a clocksource which continues to tick across
> suspend/resume.
I'm referring to read_persistent_clock64() API... which is distinct from the
CLOCK_SOURCE_SUSPEND_NONSTOP flag.
> Such clocksources can be flagged with CLOCK_SOURCE_SUSPEND_NONSTOP and the
> timekeeping resume code uses them when available instead of using the
> RTC. There are a few of them flagged as such in the kernel, but it might
...
> It's neither a problem of the timekeeping core code to select the
> appropriate clocksource for a system. That's up to the developer who
> implemented the SoC/CPU support and made a decision about rating the
> clocksource(s), which might end up selecting one which stops in suspend.
This was our first approach. However, because of some hardware
limitations we couldn't
move the system's monotonic clock to the persistent clock. They had to
be two different
clocks. (Details below.)
> Without looking at what it does, I can tell you that making it a config
> option is not going to fly. It has to be runtime discoverable as we have to
> support multi platform kernels.
OK. Here's a couple ideas...
APPROACH ONE: Use a heuristic
If read_persistent_clock64() ever returns fractional seconds (as
apposed to whole seconds), then
permanently disable the compensation.
APPROACH TWO: Use a property
Provide another weak symbol like this:
extern u64 persistent_clock_precision;
or..
void get_persistent_clock_precision(struct timespec *ts);
void get_persistent_clock_percision64(struct timespec64 *ts);
And the value returned should be, approximately, the minimum time
between clock ticks.
For clocks that only supply whole-second precision... the value should
be NSECS_PER_SEC.
For clocks that are backed by, say, a 32kHz clock... the value should be
(NSECS_PER_SEC/32768).
> Though I still want to know exactly why you think that you need some extra
> magic in the timekeeping core code. If your system has a clocksource which
> is not stopping on suspend and it lacks the flag, then this is a one liner
> patch. If there is something else, then please elaborate.
Long story short: you can't always have your low-power clock be your
monotonic/sched
clock.
The SoC we use backs the monotonic clock (sched_clock_register()) with
a counter that is
high frequency (>10 MHz) in their reference implementation. But it
does not count when the
system is in low-power mode. However, it can be configured to use a
32kHz clock that *does*
count when the system is in low-power mode. So, we started by using
this clock and setting the
CLOCK_SOURCE_SUSPEND_NONSTOP flag. It worked great... at first.
Then we found that devices would randomly experience a 36-hour time jump.
While we don't have a definitive root cause, the current theory is
that we are getting
non-atomic reads because the clock source isn't synchronized with the
the high frequency
clock (which is used for most of the digital logic on the SoC).
Therefore, we moved the monotonic/sched clock back to the high-frequency source.
Meanwhile, we can directly read the RTC clock on this system, and it
will give us 32kHz
resolution and also runs non-stop. Since reads are non-atomic, we have
to read the
registers in a loop. We used this register to implement
read_persistent_clock64().
Because we have to read the registers in a loop, it seemed unfit for use as the
monotonic/sched clock.
Thanks,
Gabe
^ permalink raw reply
* Re: Extreme time jitter with suspend/resume cycles
From: Thomas Gleixner @ 2017-10-04 18:22 UTC (permalink / raw)
To: Gabriel Beddingfield
Cc: LKML, Stephen Boyd, John Stultz, Alessandro Zummo,
Alexandre Belloni, linux-rtc, Guy Erb, hharte
In-Reply-To: <CAOdF7nvHfh_M3UQDYjbofNOm0-S_-LXHdmsMY7DeEwgOQCmfFA@mail.gmail.com>
Gabriel,
On Wed, 4 Oct 2017, Gabriel Beddingfield wrote:
thanks for bringing this up. Let's see where this goes.
Disclaimer: I did not bother to look at the patch yet because:
1) It's an attachment and I'm too lazy to open and convert it to inline
for reply. Sending patches inline is the preferred way, but maybe I
should be thankful that you did not, as you consider it as ugly
yourself.
2) I want to discuss the conceptual issues first w/o being biased by
looking at the patch.
> We have been developing a device that has very a very aggressive power
> policy, doing suspend/resume cycles a few times a minute ("echo mem >
> /sys/power/state"). In doing so, we found that the system time
> experiences a lot of jitter (compared to, say, an NTP server). It was
> not uncommon for us to see time corrections of 1s to 4s on a regular
> basis. This didn't happen when the device stayed awake, only when it
> was allowed to do suspend/resume.
>
> We found that the problem is an interaction between the NTP code and
> what I call the "delta_delta hack." (see [1] and [2])
What you consider a hack might other people not. In fact it is the least
resort the kernel has on a lot of systems to maintain some halfways
accurate notion of walltime across suspend/resume.
I personally consider using high frequency suspend to mem a hack as well,
because contrary to suspend to idle and proper runtime power management it
must go through a lot of work which drains power and it prevents the kernel
from using all of its knowledge of power saving / wakeup predictions etc to
avoid that. But it might be as well the least resort on a particular
system or use case scenario to squeeze the most lifetime out of the
battery.
Calling things a hack which might have been thought out carefully does not
make people more receptive.
> This code allocates a static variable in a function that contains an
> offset from the system time to the persistent/rtc clock. It uses that
> time to fudge the suspend timestamp so that on resume the sleep time will
> be compensated. It's kind of a statistical hack that assumes things will
> average out. It seems to have two main assumptions:
>
> 1. The persistent/rtc clock has only single-second precision
> 2. The system does not frequently suspend/resume.
> 3. If delta_delta is less than 2 seconds, these assumptions are "true"
>
> Because the delta_delta hack is trying to maintain an offset from the
> system time to the persistent/rtc clock, any minor NTP corrections
> that have occurred since the last suspend will be discarded. However,
> the NTP subsystem isn't notified that this is happening -- and so it
> causes some level of instability in its PLL logic.
The latter might be trivial to solve by adding TK_NTP_CLEAR to the
timekeeping_update() flags if the RTC was used for injecting the sleep
time, but I leave that to John for judgement.
> This problem affects any device that does "frequent" suspend/resume
> cycles. I.e. any battery-powered "linux" device (like Android phones).
Depends as there are other options depending on your SoC/CPU.
> Many ARM systems provide a "persistent clock." Most of them are backed
> by a 32kHz clock that gives good precision and makes the delta_delta
> hack unnecessary. However, devices that only have single-second
> precision for the persistent clock and/or are forced to use the RTC
> (whose API only allows for single-second precision) -- they still need
> this hack.
I have no idea what you are trying to tell me. We know that there are
systems which have a clocksource which continues to tick across
suspend/resume.
Such clocksources can be flagged with CLOCK_SOURCE_SUSPEND_NONSTOP and the
timekeeping resume code uses them when available instead of using the
RTC. There are a few of them flagged as such in the kernel, but it might
not be the complete set of capable devices which is neither a problem of
the timekeeping core code nor of the maintainers, as we are not able to
verify every single hardware detail of a submitted driver for obvious
reasons.
It's neither a problem of the timekeeping core code to select the
appropriate clocksource for a system. That's up to the developer who
implemented the SoC/CPU support and made a decision about rating the
clocksource(s), which might end up selecting one which stops in suspend.
If your particular use case is not working with the rating decision of the
developers you can override that decision on the kernel command line or
after boot through sysfs. With some restrictions you can even switch back
and forth during runtime, though that might not be the best idea if you
want to maintain a high precision wall time clock. Again, that depends on
your use case and system.
If the clocksource on your system is missing a flag despite being nonstop
across suspend, feel free to send a patch.
> I've attached a patch that we developed in-house. I have a feeling you
> won't like it... since it pushes the responsibility on whoever
> configures the kernel. It also ignores the RTC problem (which will
> still affect a lot of battery-powered devices).
Without looking at what it does, I can tell you that making it a config
option is not going to fly. It has to be runtime discoverable as we have to
support multi platform kernels.
Though I still want to know exactly why you think that you need some extra
magic in the timekeeping core code. If your system has a clocksource which
is not stopping on suspend and it lacks the flag, then this is a one liner
patch. If there is something else, then please elaborate.
Thanks,
tglx
^ permalink raw reply
* Extreme time jitter with suspend/resume cycles
From: Gabriel Beddingfield @ 2017-10-04 16:11 UTC (permalink / raw)
To: LKML, Stephen Boyd, Thomas Gleixner, John Stultz,
Alessandro Zummo, Alexandre Belloni, linux-rtc
Cc: Guy Erb, hharte
[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]
TL;DR: the "delta_delta" hack[1 and 2] in kernel/time/timekeeping.c
and drivers/rtc/class.c undermines the NTP system. It's not
appropriate to use if sub-second precision is available. I've attached
a patch to resolve this... please let me know the ways you hate it.
:-)
Hello Kernel Timekeeping Maintainers,
We have been developing a device that has very a very aggressive power
policy, doing suspend/resume cycles a few times a minute ("echo mem >
/sys/power/state"). In doing so, we found that the system time
experiences a lot of jitter (compared to, say, an NTP server). It was
not uncommon for us to see time corrections of 1s to 4s on a regular
basis. This didn't happen when the device stayed awake, only when it
was allowed to do suspend/resume.
We found that the problem is an interaction between the NTP code and
what I call the "delta_delta hack." (see [1] and [2]) This code
allocates a static variable in a function that contains an offset from
the system time to the persistent/rtc clock. It uses that time to
fudge the suspend timestamp so that on resume the sleep time will be
compensated. It's kind of a statistical hack that assumes things will
average out. It seems to have two main assumptions:
1. The persistent/rtc clock has only single-second precision
2. The system does not frequently suspend/resume.
3. If delta_delta is less than 2 seconds, these assumptions are "true"
Because the delta_delta hack is trying to maintain an offset from the
system time to the persistent/rtc clock, any minor NTP corrections
that have occurred since the last suspend will be discarded. However,
the NTP subsystem isn't notified that this is happening -- and so it
causes some level of instability in its PLL logic.
This problem affects any device that does "frequent" suspend/resume
cycles. I.e. any battery-powered "linux" device (like Android phones).
Many ARM systems provide a "persistent clock." Most of them are backed
by a 32kHz clock that gives good precision and makes the delta_delta
hack unnecessary. However, devices that only have single-second
precision for the persistent clock and/or are forced to use the RTC
(whose API only allows for single-second precision) -- they still need
this hack.
I've attached a patch that we developed in-house. I have a feeling you
won't like it... since it pushes the responsibility on whoever
configures the kernel. It also ignores the RTC problem (which will
still affect a lot of battery-powered devices).
Please let me know what you think -- and what the right approach for
solving this would be.
Thanks,
Gabe
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/kernel/time/timekeeping.c?h=v4.13.4#n1717
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/rtc/class.c?h=v4.13.4#n76
[-- Attachment #2: 0001-time-add-CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION-to.patch --]
[-- Type: text/x-patch, Size: 2565 bytes --]
From c03ceced9a210b48f2552e7dafa9099ef2449370 Mon Sep 17 00:00:00 2001
From: "Gabriel M. Beddingfield" <beddingfield@nestlabs.com>
Date: Wed, 4 Oct 2017 08:38:57 -0700
Subject: [PATCH] time: add CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION to disable
suspend/resume hack
Signed-off-by: Gabriel M. Beddingfield <beddingfield@nestlabs.com>
Signed-off-by: Guy <guy@nestlabs.com>
---
kernel/time/Kconfig | 16 ++++++++++++++++
kernel/time/timekeeping.c | 4 ++++
2 files changed, 20 insertions(+)
diff --git a/kernel/time/Kconfig b/kernel/time/Kconfig
index ac09bc29eb08..32d54086c96c 100644
--- a/kernel/time/Kconfig
+++ b/kernel/time/Kconfig
@@ -143,5 +143,21 @@ config HIGH_RES_TIMERS
hardware is not capable then this option only increases
the size of the kernel image.
+config PERSISTENT_CLOCK_IS_LOW_PRECISION
+ bool "The persistent clock has only single-second precision"
+ default y
+ help
+ When enabled, then on suspend/resume the timekeeping code will
+ try to maintain a constant offset between the system time and
+ the persistent clock as a means of compensating for the coarse
+ (+/- 1 second) sleep time calculation. However, this will discard
+ any "small" NTP corrections that have happened since the last
+ resume. However, if the system's persistent clock has better
+ precision (e.g. because it's backed by a 32kHz clock), this is
+ not necessary and introduces unneeded time jitter.
+
+ If your persistent clock has only single-second precision, say Y.
+ If your persistent clock has sub-second precision, say N.
+
endmenu
endif
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 2cafb49aa65e..b2c7b443ef37 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1694,8 +1694,10 @@ int timekeeping_suspend(void)
{
struct timekeeper *tk = &tk_core.timekeeper;
unsigned long flags;
+#ifdef CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION
struct timespec64 delta, delta_delta;
static struct timespec64 old_delta;
+#endif
read_persistent_clock64(&timekeeping_suspend_time);
@@ -1712,6 +1714,7 @@ int timekeeping_suspend(void)
timekeeping_forward_now(tk);
timekeeping_suspended = 1;
+#ifdef CONFIG_PERSISTENT_CLOCK_IS_LOW_PRECISION
if (persistent_clock_exists) {
/*
* To avoid drift caused by repeated suspend/resumes,
@@ -1733,6 +1736,7 @@ int timekeeping_suspend(void)
timespec64_add(timekeeping_suspend_time, delta_delta);
}
}
+#endif
timekeeping_update(tk, TK_MIRROR);
halt_fast_timekeeper(tk);
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related
* Re: [RFC v3 3/7] platform/x86: intel_pmc_ipc: Use regmap calls for GCR updates
From: Andy Shevchenko @ 2017-10-04 12:37 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <5ba5b3d9-b55a-5834-04e5-b4b9a356dca7@linux.intel.com>
On Wed, Oct 4, 2017 at 4:16 AM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> On 10/01/2017 07:48 AM, Andy Shevchenko wrote:
>> On Tue, Sep 5, 2017 at 8:37 AM,
>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Since it sounds as candidate for stable,
>
> Yes.
>>
>> can we have split it to just
>> as less as possible intrusive fix + moving to regmap as a separate
>> change?
>
> If we have to split it into two patches then,
>
> Patch #1 will fix the "sleep in atomic context issue" by replacing
> mutex_lock() with spin_lock()
> in GCR read/write APIs to protect the GCR memory updates.
> Patch #2 will remove GCR read/write/update APIs and replace it with regmap
> APIs. But along with this
> change we will also remove the spin_lock() added in previous patch because
> regmap calls are already
> protected by its own locking mechanism.
>
> Since Patch #2 will clean up what we do in Patch #1, Do we need to split it
> into two patches?
Yes, please do...
>> It should include Fixes: tag as well I suppose.
>
> Agree. I will add Fixes tag in next version.
...because this one will go alone to stable releases.
Be also sure patch #1 will be applied on current vanilla (w/o PDx86
queue involvement).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v3 2/7] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: Andy Shevchenko @ 2017-10-04 12:34 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <6b854d35-6430-c790-3608-15cefac03a52@linux.intel.com>
On Wed, Oct 4, 2017 at 4:00 AM, sathyanarayanan kuppuswamy
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> On 10/01/2017 07:44 AM, Andy Shevchenko wrote:
>> On Tue, Sep 5, 2017 at 8:37 AM,
>>> + punit_cell.id = -1;
>
> I will remove this line in next version.
>>>
>>> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
>>> + &punit_cell, 1, NULL, 0, NULL);
>>
>> IIRC you don't need to file cell ID in case of DEVID_AUTO.
>
> I am planning to use DEVID_NONE here to match the current behavior. Unless
> you have some concerns?
Preventing behaviour is a good thing to do.
In either way cell.id can be leaved 0.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v3 3/7] platform/x86: intel_pmc_ipc: Use regmap calls for GCR updates
From: sathyanarayanan kuppuswamy @ 2017-10-04 1:16 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <CAHp75VdGEf_e6mopQmFG5+Dv3X1f5TwV8cHn2CjA39USqN1JBw@mail.gmail.com>
Hi Andy,
On 10/01/2017 07:48 AM, Andy Shevchenko wrote:
> On Tue, Sep 5, 2017 at 8:37 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Currently, update_no_reboot_bit() function implemented in this driver
>> uses mutex_lock to protect its register updates. But this function is
>> called with in atomic context in iTCO_wdt_start() and iTCO_wdt_stop()
>> functions in iTCO_wdt.c driver, which in turn causes "sleeping into
>> atomic context" issue. This patch fixes this issue by refactoring the
>> current GCR read/write/update functions with regmap APIs.
> Since it sounds as candidate for stable,
Yes.
> can we have split it to just
> as less as possible intrusive fix + moving to regmap as a separate
> change?
If we have to split it into two patches then,
Patch #1 will fix the "sleep in atomic context issue" by replacing
mutex_lock() with spin_lock()
in GCR read/write APIs to protect the GCR memory updates.
Patch #2 will remove GCR read/write/update APIs and replace it with
regmap APIs. But along with this
change we will also remove the spin_lock() added in previous patch
because regmap calls are already
protected by its own locking mechanism.
Since Patch #2 will clean up what we do in Patch #1, Do we need to split
it into two patches?
> It should include Fixes: tag as well I suppose.
Agree. I will add Fixes tag in next version.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> drivers/platform/x86/Kconfig | 1 +
>> drivers/platform/x86/intel_pmc_ipc.c | 115 ++++++++++++-----------------------
>> 2 files changed, 40 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 80b8795..45f4e79 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -1054,6 +1054,7 @@ config PVPANIC
>> config INTEL_PMC_IPC
>> tristate "Intel PMC IPC Driver"
>> depends on ACPI
>> + select REGMAP_MMIO
>> ---help---
>> This driver provides support for PMC control on some Intel platforms.
>> The PMC is an ARC processor which defines IPC commands for communication
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index 021dcf6..40a25f8 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -31,9 +31,11 @@
>> #include <linux/atomic.h>
>> #include <linux/notifier.h>
>> #include <linux/suspend.h>
>> +#include <linux/spinlock.h>
>> #include <linux/acpi.h>
>> #include <linux/io-64-nonatomic-lo-hi.h>
>> #include <linux/mfd/core.h>
>> +#include <linux/regmap.h>
>>
>> #include <asm/intel_pmc_ipc.h>
>>
>> @@ -125,7 +127,7 @@ static struct intel_pmc_ipc_dev {
>>
>> /* gcr */
>> void __iomem *gcr_mem_base;
>> - bool has_gcr_regs;
>> + struct regmap *gcr_regs;
>>
>> /* Telemetry */
>> u8 telem_res_inval;
>> @@ -150,6 +152,14 @@ static char *ipc_err_sources[] = {
>> "Unsigned kernel",
>> };
>>
>> +static struct regmap_config gcr_regmap_config = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> + .fast_io = true,
>> + .max_register = PLAT_RESOURCE_GCR_SIZE,
>> +};
>> +
>> /* Prevent concurrent calls to the PMC */
>> static DEFINE_MUTEX(ipclock);
>>
>> @@ -183,21 +193,6 @@ static inline u32 ipc_data_readl(u32 offset)
>> return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
>> }
>>
>> -static inline u64 gcr_data_readq(u32 offset)
>> -{
>> - return readq(ipcdev.gcr_mem_base + offset);
>> -}
>> -
>> -static inline int is_gcr_valid(u32 offset)
>> -{
>> - if (!ipcdev.has_gcr_regs)
>> - return -EACCES;
>> -
>> - if (offset > PLAT_RESOURCE_GCR_SIZE)
>> - return -EINVAL;
>> -
>> - return 0;
>> -}
>>
>> /**
>> * intel_pmc_gcr_read() - Read PMC GCR register
>> @@ -210,21 +205,10 @@ static inline int is_gcr_valid(u32 offset)
>> */
>> int intel_pmc_gcr_read(u32 offset, u32 *data)
>> {
>> - int ret;
>> -
>> - mutex_lock(&ipclock);
>> -
>> - ret = is_gcr_valid(offset);
>> - if (ret < 0) {
>> - mutex_unlock(&ipclock);
>> - return ret;
>> - }
>> -
>> - *data = readl(ipcdev.gcr_mem_base + offset);
>> -
>> - mutex_unlock(&ipclock);
>> + if (!ipcdev.gcr_regs)
>> + return -EACCES;
>>
>> - return 0;
>> + return regmap_read(ipcdev.gcr_regs, offset, data);
>> }
>> EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>>
>> @@ -240,21 +224,10 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>> */
>> int intel_pmc_gcr_write(u32 offset, u32 data)
>> {
>> - int ret;
>> -
>> - mutex_lock(&ipclock);
>> -
>> - ret = is_gcr_valid(offset);
>> - if (ret < 0) {
>> - mutex_unlock(&ipclock);
>> - return ret;
>> - }
>> -
>> - writel(data, ipcdev.gcr_mem_base + offset);
>> -
>> - mutex_unlock(&ipclock);
>> + if (!ipcdev.gcr_regs)
>> + return -EACCES;
>>
>> - return 0;
>> + return regmap_write(ipcdev.gcr_regs, offset, data);
>> }
>> EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>>
>> @@ -271,33 +244,10 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>> */
>> int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
>> {
>> - u32 new_val;
>> - int ret = 0;
>> -
>> - mutex_lock(&ipclock);
>> -
>> - ret = is_gcr_valid(offset);
>> - if (ret < 0)
>> - goto gcr_ipc_unlock;
>> -
>> - new_val = readl(ipcdev.gcr_mem_base + offset);
>> -
>> - new_val &= ~mask;
>> - new_val |= val & mask;
>> -
>> - writel(new_val, ipcdev.gcr_mem_base + offset);
>> -
>> - new_val = readl(ipcdev.gcr_mem_base + offset);
>> -
>> - /* check whether the bit update is successful */
>> - if ((new_val & mask) != (val & mask)) {
>> - ret = -EIO;
>> - goto gcr_ipc_unlock;
>> - }
>> + if (!ipcdev.gcr_regs)
>> + return -EACCES;
>>
>> -gcr_ipc_unlock:
>> - mutex_unlock(&ipclock);
>> - return ret;
>> + return regmap_update_bits(ipcdev.gcr_regs, offset, mask, val);
>> }
>> EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>>
>> @@ -776,16 +726,24 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>> int intel_pmc_s0ix_counter_read(u64 *data)
>> {
>> u64 deep, shlw;
>> + int ret;
>>
>> - if (!ipcdev.has_gcr_regs)
>> + if (!ipcdev.gcr_regs)
>> return -EACCES;
>>
>> - deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
>> - shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
>> + ret = regmap_bulk_read(ipcdev.gcr_regs, PMC_GCR_TELEM_DEEP_S0IX_REG,
>> + &deep, 2);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_bulk_read(ipcdev.gcr_regs, PMC_GCR_TELEM_SHLW_S0IX_REG,
>> + &shlw, 2);
>> + if (ret)
>> + return ret;
>>
>> *data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>>
>> - return 0;
>> + return ret;
>> }
>> EXPORT_SYMBOL_GPL(intel_pmc_s0ix_counter_read);
>>
>> @@ -817,6 +775,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> + ipcdev.gcr_regs = devm_regmap_init_mmio_clk(ipcdev.dev, NULL,
>> + ipcdev.gcr_mem_base, &gcr_regmap_config);
>> + if (IS_ERR(ipcdev.gcr_regs)) {
>> + dev_err(ipcdev.dev, "gcr_regs regmap init failed\n");
>> + return PTR_ERR(ipcdev.gcr_regs);;
>> + }
>> +
>> ret = ipc_create_pmc_devices(pdev);
>> if (ret) {
>> dev_err(&pdev->dev, "Failed to create pmc devices\n");
>> @@ -836,8 +801,6 @@ static int ipc_plat_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> - ipcdev.has_gcr_regs = true;
>> -
>> return 0;
>> }
>>
>> --
>> 2.7.4
>>
>
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [RFC v3 4/7] platform: x86: Add generic Intel IPC driver
From: sathyanarayanan kuppuswamy @ 2017-10-04 1:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <CAHp75VecDGzs=esey6hSDWMxvYPeeiy5T-9x6MNFaHspLq0ctA@mail.gmail.com>
Hi,
On 10/01/2017 07:59 AM, Andy Shevchenko wrote:
> On Tue, Sep 5, 2017 at 8:37 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Currently intel_scu_ipc.c, intel_pmc_ipc.c and intel_punit_ipc.c
>> redundantly implements the same IPC features and has lot of code
>> duplication between them. This driver addresses this issue by grouping
>> the common IPC functionalities under the same driver.
>> +static const char *ipc_dev_err_string(struct intel_ipc_dev *ipc_dev,
>> + int error)
>> +{
>> + switch (error) {
>> + case IPC_DEV_ERR_NONE:
>> + return "No error";
>> + case IPC_DEV_ERR_CMD_NOT_SUPPORTED:
>> + return "Command not-supported/Invalid";
>> + case IPC_DEV_ERR_CMD_NOT_SERVICED:
>> + return "Command not-serviced/Invalid param";
>> + case IPC_DEV_ERR_UNABLE_TO_SERVICE:
>> + return "Unable-to-service/Cmd-timeout";
>> + case IPC_DEV_ERR_CMD_INVALID:
>> + return "Command-invalid/Cmd-locked";
>> + case IPC_DEV_ERR_CMD_FAILED:
>> + return "Command-failed/Invalid-VR-id";
>> + case IPC_DEV_ERR_EMSECURITY:
>> + return "Invalid Battery/VR-Error";
>> + case IPC_DEV_ERR_UNSIGNEDKERNEL:
>> + return "Unsigned kernel";
>> + default:
>> + return "Unknown Command";
>> + };
>> +}
> Since it's continuous list you can define an array of messages like
>
> const char * const *errors = {
> [..._ERR_...] = "",
> ...
>
> };
>
> Also you can use enum in the header and define _ERR_MAX there.
> Thus, code would be transformed to
>
> if (error < _ERR_MAX)
> return errors[error];
>
> return "Unknown Command";
Thanks will fix it in next version.
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [RFC v3 0/7] PMC/PUNIT IPC driver cleanup
From: sathyanarayanan kuppuswamy @ 2017-10-04 1:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <CAHp75VcRGqb86cfggF9zFtfzkZkx7CSH9-nYQ2rk_G8Rgn2t7w@mail.gmail.com>
Hi Andy,
Thanks for the review. I really appreciate you taking the time to review
such big chunk of code changes.
On 10/01/2017 07:46 AM, Andy Shevchenko wrote:
> On Tue, Sep 5, 2017 at 8:37 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Hi All,
>>
>> Currently intel_pmc_ipc.c, intel_punit_ipc.c, intel_scu_ipc.c drivers implements the same IPC features.
>> This code duplication could be avoided if we implement the IPC driver as a generic library and let custom
>> device drivers use API provided by generic driver. This patchset mainly addresses this issue.
>>
>> Along with above code duplication issue, This patchset also addresses following issues in intel_pmc_ipc and
>> intel_punit_ipc drivers.
>>
>> 1. Intel_pmc_ipc.c driver does not use any resource managed (devm_*) calls.
>> 2. In Intel_pmc_ipc.c driver, dependent devices like PUNIT, Telemetry and iTCO are created manually and uses lot of redundant buffer code.
>> 3. Global variable is used to store the IPC device structure and it is used across all functions in intel_pmc_ipc.c and intel_punit_ipc.c.
>>
>> More info on Intel IPC device library:
>> -------------------------------------
>>
>> A generic Intel IPC class driver has been implemented and all common IPC helper functions has been moved to this driver. It exposes APIs to create IPC device channel, send raw IPC command and simple IPC commands. It also creates device attribute to send IPC command from user space.
>>
>> API for creating a new IPC channel device is,
>>
>> struct intel_ipc_dev *devm_intel_ipc_dev_create(struct device *dev, const char *devname, struct intel_ipc_dev_cfg *cfg, struct intel_ipc_dev_ops *ops)
>>
>> The IPC channel drivers (PUNIT/PMC/SCU) when creating a new device can configure their device params like register mapping, irq, irq-mode, channel type,etc using intel_ipc_dev_cfg and intel_ipc_dev_ops arguments. After a new IPC channel device is created, IPC users can use the generic APIs to make IPC calls.
>>
>> For example, after using this new model, IPC call to PMC device will look like,
>>
>> pmc_ipc_dev = intel_ipc_dev_get(INTEL_PMC_IPC_DEV);
>> ipc_dev_raw_cmd(pmc_ipc_dev, cmd, PMC_PARAM_LEN, (u32 *)ipc_in, 1, NULL, 0, 0, 0);
>>
>> I am still testing the driver in different products. But posted it to get some early comments. I also welcome any PMC/PUNIT driver users to check these patches in their product.
> I have applied first patch from the series with some small changes.
> Please rebase the rest on top of my review branch (review-andy).
Will do. I will soon send v4 version.
> Before sending new version, address comments that others and me did.
Yes.
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [RFC v3 2/7] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: sathyanarayanan kuppuswamy @ 2017-10-04 1:00 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <CAHp75Vd4wNg9tpRUHpkk8-Y75xXtPe4F7_27dPQHZT9uN9DJ5g@mail.gmail.com>
Hi Andy,
On 10/01/2017 07:44 AM, Andy Shevchenko wrote:
> On Tue, Sep 5, 2017 at 8:37 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Currently, we have lot of repetitive code in dependent device resource
>> allocation and device creation handling code. This logic can be improved if
>> we use MFD framework for dependent device creation. This patch adds this
>> support.
>>
>> + punit_cell.id = -1;
I will remove this line in next version.
>> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
>> + &punit_cell, 1, NULL, 0, NULL);
> IIRC you don't need to file cell ID in case of DEVID_AUTO.
I am planning to use DEVID_NONE here to match the current behavior.
Unless you have some concerns?
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [RFC v3 1/7] platform/x86: intel_pmc_ipc: Use devm_* calls in driver probe function
From: sathyanarayanan kuppuswamy @ 2017-10-04 0:55 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <CAHp75VdXP+R=HrbicYSWTyLMZQWf54tmgviVW7J_+hhJBPkF1A@mail.gmail.com>
Hi,
On 10/01/2017 07:34 AM, Andy Shevchenko wrote:
> On Tue, Sep 5, 2017 at 8:37 AM,
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> This patch cleans up unnecessary free/alloc calls in ipc_plat_probe(),
>> ipc_pci_probe() and ipc_plat_get_res() functions by using devm_*
>> calls.
>>
>> This patch also adds proper error handling for failure cases in
>> ipc_pci_probe() function.
>>
> Pushed (this patch only) to my review queue with slight style changes, thanks.
Thanks Andy. Will rebase my rest of the patches on top of this patch.
>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>> drivers/platform/x86/intel_pmc_ipc.c | 104 ++++++++++++-----------------------
>> 1 file changed, 34 insertions(+), 70 deletions(-)
>>
>> Changes since v2:
>> * Used pcim_* device managed functions.
>>
>> Changes since v1:
>> * Merged devm_* related changes into a single function.
>> * Instead of removing free_irq, use devm_free_irq function.
>>
>> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
>> index bb792a5..5eef649 100644
>> --- a/drivers/platform/x86/intel_pmc_ipc.c
>> +++ b/drivers/platform/x86/intel_pmc_ipc.c
>> @@ -480,52 +480,39 @@ static irqreturn_t ioc(int irq, void *dev_id)
>>
>> static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> - resource_size_t pci_resource;
>> int ret;
>> - int len;
>> + struct intel_pmc_ipc_dev *pmc = &ipcdev;
>>
>> - ipcdev.dev = &pci_dev_get(pdev)->dev;
>> - ipcdev.irq_mode = IPC_TRIGGER_MODE_IRQ;
>> + /* Only one PMC is supported */
>> + if (pmc->dev)
>> + return -EBUSY;
>>
>> - ret = pci_enable_device(pdev);
>> + pmc->irq_mode = IPC_TRIGGER_MODE_IRQ;
>> +
>> + ret = pcim_enable_device(pdev);
>> if (ret)
>> return ret;
>>
>> - ret = pci_request_regions(pdev, "intel_pmc_ipc");
>> + ret = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
>> if (ret)
>> return ret;
>>
>> - pci_resource = pci_resource_start(pdev, 0);
>> - len = pci_resource_len(pdev, 0);
>> - if (!pci_resource || !len) {
>> - dev_err(&pdev->dev, "Failed to get resource\n");
>> - return -ENOMEM;
>> - }
>> + init_completion(&pmc->cmd_complete);
>>
>> - init_completion(&ipcdev.cmd_complete);
>> + pmc->ipc_base = pcim_iomap_table(pdev)[0];
>>
>> - if (request_irq(pdev->irq, ioc, 0, "intel_pmc_ipc", &ipcdev)) {
>> + ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
>> + pmc);
>> + if (ret) {
>> dev_err(&pdev->dev, "Failed to request irq\n");
>> - return -EBUSY;
>> + return ret;
>> }
>>
>> - ipcdev.ipc_base = ioremap_nocache(pci_resource, len);
>> - if (!ipcdev.ipc_base) {
>> - dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
>> - free_irq(pdev->irq, &ipcdev);
>> - ret = -ENOMEM;
>> - }
>> + pmc->dev = &pdev->dev;
>>
>> - return ret;
>> -}
>> + pci_set_drvdata(pdev, pmc);
>>
>> -static void ipc_pci_remove(struct pci_dev *pdev)
>> -{
>> - free_irq(pdev->irq, &ipcdev);
>> - pci_release_regions(pdev);
>> - pci_dev_put(pdev);
>> - iounmap(ipcdev.ipc_base);
>> - ipcdev.dev = NULL;
>> + return 0;
>> }
>>
>> static const struct pci_device_id ipc_pci_ids[] = {
>> @@ -540,7 +527,6 @@ static struct pci_driver ipc_pci_driver = {
>> .name = "intel_pmc_ipc",
>> .id_table = ipc_pci_ids,
>> .probe = ipc_pci_probe,
>> - .remove = ipc_pci_remove,
>> };
>>
>> static ssize_t intel_pmc_ipc_simple_cmd_store(struct device *dev,
>> @@ -849,18 +835,16 @@ static int ipc_plat_get_res(struct platform_device *pdev)
>> dev_err(&pdev->dev, "Failed to get ipc resource\n");
>> return -ENXIO;
>> }
>> - size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
>> + res->end = (res->start + PLAT_RESOURCE_IPC_SIZE +
>> + PLAT_RESOURCE_GCR_SIZE - 1);
>>
>> - if (!request_mem_region(res->start, size, pdev->name)) {
>> - dev_err(&pdev->dev, "Failed to request ipc resource\n");
>> - return -EBUSY;
>> - }
>> - addr = ioremap_nocache(res->start, size);
>> - if (!addr) {
>> - dev_err(&pdev->dev, "I/O memory remapping failed\n");
>> - release_mem_region(res->start, size);
>> - return -ENOMEM;
>> + addr = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(addr)) {
>> + dev_err(&pdev->dev,
>> + "PMC I/O memory remapping failed\n");
>> + return PTR_ERR(addr);
>> }
>> +
>> ipcdev.ipc_base = addr;
>>
>> ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
>> @@ -917,7 +901,6 @@ MODULE_DEVICE_TABLE(acpi, ipc_acpi_ids);
>>
>> static int ipc_plat_probe(struct platform_device *pdev)
>> {
>> - struct resource *res;
>> int ret;
>>
>> ipcdev.dev = &pdev->dev;
>> @@ -939,61 +922,42 @@ static int ipc_plat_probe(struct platform_device *pdev)
>> ret = ipc_create_pmc_devices();
>> if (ret) {
>> dev_err(&pdev->dev, "Failed to create pmc devices\n");
>> - goto err_device;
>> + return ret;
>> }
>>
>> - if (request_irq(ipcdev.irq, ioc, IRQF_NO_SUSPEND,
>> - "intel_pmc_ipc", &ipcdev)) {
>> + if (devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
>> + "intel_pmc_ipc", &ipcdev)) {
>> dev_err(&pdev->dev, "Failed to request irq\n");
>> ret = -EBUSY;
>> - goto err_irq;
>> + goto unregister_devices;
>> }
>>
>> ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
>> if (ret) {
>> dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
>> ret);
>> - goto err_sys;
>> + goto unregister_devices;
>> }
>>
>> ipcdev.has_gcr_regs = true;
>>
>> return 0;
>> -err_sys:
>> - free_irq(ipcdev.irq, &ipcdev);
>> -err_irq:
>> +
>> +unregister_devices:
>> platform_device_unregister(ipcdev.tco_dev);
>> platform_device_unregister(ipcdev.punit_dev);
>> platform_device_unregister(ipcdev.telemetry_dev);
>> -err_device:
>> - iounmap(ipcdev.ipc_base);
>> - res = platform_get_resource(pdev, IORESOURCE_MEM,
>> - PLAT_RESOURCE_IPC_INDEX);
>> - if (res) {
>> - release_mem_region(res->start,
>> - PLAT_RESOURCE_IPC_SIZE +
>> - PLAT_RESOURCE_GCR_SIZE);
>> - }
>> +
>> return ret;
>> }
>>
>> static int ipc_plat_remove(struct platform_device *pdev)
>> {
>> - struct resource *res;
>> -
>> sysfs_remove_group(&pdev->dev.kobj, &intel_ipc_group);
>> - free_irq(ipcdev.irq, &ipcdev);
>> + devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
>> platform_device_unregister(ipcdev.tco_dev);
>> platform_device_unregister(ipcdev.punit_dev);
>> platform_device_unregister(ipcdev.telemetry_dev);
>> - iounmap(ipcdev.ipc_base);
>> - res = platform_get_resource(pdev, IORESOURCE_MEM,
>> - PLAT_RESOURCE_IPC_INDEX);
>> - if (res) {
>> - release_mem_region(res->start,
>> - PLAT_RESOURCE_IPC_SIZE +
>> - PLAT_RESOURCE_GCR_SIZE);
>> - }
>> ipcdev.dev = NULL;
>> return 0;
>> }
>> --
>> 2.7.4
>>
>
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [RFC v3 7/7] platform/x86: intel_scu_ipc: Use generic Intel IPC device calls
From: sathyanarayanan kuppuswamy @ 2017-10-04 0:55 UTC (permalink / raw)
To: Guenter Roeck, Alexandre Belloni
Cc: a.zummo, x86, wim, mingo, qipeng.zha, hpa, dvhart, tglx,
lee.jones, andy, souvik.k.chakravarty, linux-rtc, linux-watchdog,
linux-kernel, platform-driver-x86, sathyaosid
In-Reply-To: <c8f64c42-9802-9746-3032-b932522659e5@roeck-us.net>
Hi Guenter,
On 09/28/2017 06:35 AM, Guenter Roeck wrote:
> On 09/28/2017 05:55 AM, Alexandre Belloni wrote:
>> On 04/09/2017 at 22:37:27 -0700,
>> sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>> From: Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>
>>> Removed redundant IPC helper functions and refactored the driver to use
>>> generic IPC device driver APIs.
>>>
>>> This patch also cleans-up SCU IPC user drivers to use APIs provided
>>> by generic IPC driver.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> For the RTC part:
>> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>> >
>>> ---
>>> arch/x86/include/asm/intel_scu_ipc.h | 23 +-
>>> arch/x86/platform/intel-mid/intel-mid.c | 12 +-
>>> drivers/platform/x86/Kconfig | 1 +
>>> drivers/platform/x86/intel_scu_ipc.c | 483
>>> +++++++++++++-------------------
>>> drivers/rtc/rtc-mrst.c | 15 +-
>>> drivers/watchdog/intel-mid_wdt.c | 12 +-
>>> drivers/watchdog/intel_scu_watchdog.c | 17 +-
>>> 7 files changed, 251 insertions(+), 312 deletions(-)
>
> I think it would help in general to mention at least in the
> description which drivers are touched.
I will add this info in my next patch version.
>
> I see calls to intel_ipc_dev_get() but not associated put calls.
> Missing the context, it may
> well be that those are not necessary, but then how does the ipc code
> know that the device
> reference is no longer needed ?
I also noticed this issue. Initially I thought we don't need to deal
with ref count because in most
cases IPC client drivers are compiled as built-in drivers ( because
these APIs are required during
early boot process to send IPC commands to ARC processor) and hence will
have life-time until the
device is powered-off.
But after looking into Kconfig params, I noticed that at least
intel_pmc_ipc and intel_punit_ipc drivers can be
compiled as modules. And hence we will have dangling pointer reference
issue if we don't maintain
proper refcount for device references.
So, I have already fixed this problem by adding intel_ipc_dev_put() and
devm_intel_ipc_dev_get() APIs.
This fix will be available in next version.
> Guenter
>
>>>
>>> diff --git a/arch/x86/include/asm/intel_scu_ipc.h
>>> b/arch/x86/include/asm/intel_scu_ipc.h
>>> index 81d3d87..5842534 100644
>>> --- a/arch/x86/include/asm/intel_scu_ipc.h
>>> +++ b/arch/x86/include/asm/intel_scu_ipc.h
>>> @@ -14,9 +14,19 @@
>>> #define IPCMSG_COLD_BOOT 0xF3
>>> #define IPCMSG_VRTC 0xFA /* Set vRTC device */
>>> - /* Command id associated with message IPCMSG_VRTC */
>>> - #define IPC_CMD_VRTC_SETTIME 1 /* Set time */
>>> - #define IPC_CMD_VRTC_SETALARM 2 /* Set alarm */
>>> +
>>> +/* Command id associated with message IPCMSG_VRTC */
>>> +#define IPC_CMD_VRTC_SETTIME 1 /* Set time */
>>> +#define IPC_CMD_VRTC_SETALARM 2 /* Set alarm */
>>> +
>>> +#define INTEL_SCU_IPC_DEV "intel_scu_ipc"
>>> +#define SCU_PARAM_LEN 2
>>> +
>>> +static inline void scu_cmd_init(u32 *cmd, u32 param1, u32 param2)
>>> +{
>>> + cmd[0] = param1;
>>> + cmd[1] = param2;
>>> +}
>>> /* Read single register */
>>> int intel_scu_ipc_ioread8(u16 addr, u8 *data);
>>> @@ -45,13 +55,6 @@ int intel_scu_ipc_writev(u16 *addr, u8 *data, int
>>> len);
>>> /* Update single register based on the mask */
>>> int intel_scu_ipc_update_register(u16 addr, u8 data, u8 mask);
>>> -/* Issue commands to the SCU with or without data */
>>> -int intel_scu_ipc_simple_command(int cmd, int sub);
>>> -int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
>>> - u32 *out, int outlen);
>>> -int intel_scu_ipc_raw_command(int cmd, int sub, u8 *in, int inlen,
>>> - u32 *out, int outlen, u32 dptr, u32 sptr);
>>> -
>>> /* I2C control api */
>>> int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data);
>>> diff --git a/arch/x86/platform/intel-mid/intel-mid.c
>>> b/arch/x86/platform/intel-mid/intel-mid.c
>>> index 12a2725..27541e9 100644
>>> --- a/arch/x86/platform/intel-mid/intel-mid.c
>>> +++ b/arch/x86/platform/intel-mid/intel-mid.c
>>> @@ -22,6 +22,7 @@
>>> #include <linux/irq.h>
>>> #include <linux/export.h>
>>> #include <linux/notifier.h>
>>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>> #include <asm/setup.h>
>>> #include <asm/mpspec_def.h>
>>> @@ -68,18 +69,23 @@ static void *(*get_intel_mid_ops[])(void) =
>>> INTEL_MID_OPS_INIT;
>>> enum intel_mid_cpu_type __intel_mid_cpu_chip;
>>> EXPORT_SYMBOL_GPL(__intel_mid_cpu_chip);
>>> +static struct intel_ipc_dev *ipc_dev;
>>> +
>>> static void intel_mid_power_off(void)
>>> {
>>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_COLD_OFF, 1};
>>> /* Shut down South Complex via PWRMU */
>>> intel_mid_pwr_power_off();
>>> /* Only for Tangier, the rest will ignore this command */
>>> - intel_scu_ipc_simple_command(IPCMSG_COLD_OFF, 1);
>>> + ipc_dev_simple_cmd(ipc_dev, cmds, SCU_PARAM_LEN);
>>> };
>>> static void intel_mid_reboot(void)
>>> {
>>> - intel_scu_ipc_simple_command(IPCMSG_COLD_BOOT, 0);
>>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_COLD_BOOT, 0};
>>> +
>>> + ipc_dev_simple_cmd(ipc_dev, cmds, SCU_PARAM_LEN);
>>> }
>>> static unsigned long __init intel_mid_calibrate_tsc(void)
>>> @@ -206,6 +212,8 @@ void __init x86_intel_mid_early_setup(void)
>>> x86_init.mpparse.find_smp_config = x86_init_noop;
>>> x86_init.mpparse.get_smp_config = x86_init_uint_noop;
>>> set_bit(MP_BUS_ISA, mp_bus_not_pci);
>>> +
>>> + ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>>> }
>>> /*
>>> diff --git a/drivers/platform/x86/Kconfig
>>> b/drivers/platform/x86/Kconfig
>>> index 82479ca..e4e5822 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -850,6 +850,7 @@ config INTEL_VBTN
>>> config INTEL_SCU_IPC
>>> bool "Intel SCU IPC Support"
>>> depends on X86_INTEL_MID
>>> + select REGMAP_MMIO
>>> default y
>>> ---help---
>>> IPC is used to bridge the communications between kernel and
>>> SCU on
>>> diff --git a/drivers/platform/x86/intel_scu_ipc.c
>>> b/drivers/platform/x86/intel_scu_ipc.c
>>> index f7cf981..78013e4 100644
>>> --- a/drivers/platform/x86/intel_scu_ipc.c
>>> +++ b/drivers/platform/x86/intel_scu_ipc.c
>>> @@ -24,6 +24,8 @@
>>> #include <linux/pci.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/sfi.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>> #include <asm/intel-mid.h>
>>> #include <asm/intel_scu_ipc.h>
>>> @@ -39,6 +41,25 @@
>>> #define IPC_CMD_PCNTRL_R 1 /* Register read */
>>> #define IPC_CMD_PCNTRL_M 2 /* Register read-modify-write */
>>> +/* IPC dev register offsets */
>>> +/*
>>> + * IPC Read Buffer (Read Only):
>>> + * 16 byte buffer for receiving data from SCU, if IPC command
>>> + * processing results in response data
>>> + */
>>> +#define IPC_DEV_SCU_RBUF_OFFSET 0x90
>>> +#define IPC_DEV_SCU_WRBUF_OFFSET 0x80
>>> +#define IPC_DEV_SCU_SPTR_OFFSET 0x08
>>> +#define IPC_DEV_SCU_DPTR_OFFSET 0x0C
>>> +#define IPC_DEV_SCU_STATUS_OFFSET 0x04
>>> +
>>> +/* IPC dev commands */
>>> +/* IPC command register IOC bit */
>>> +#define IPC_DEV_SCU_CMD_MSI BIT(8)
>>> +#define IPC_DEV_SCU_CMD_STATUS_ERR BIT(1)
>>> +#define IPC_DEV_SCU_CMD_STATUS_ERR_MASK GENMASK(7, 0)
>>> +#define IPC_DEV_SCU_CMD_STATUS_BUSY BIT(0)
>>> +
>>> /*
>>> * IPC register summary
>>> *
>>> @@ -59,6 +80,11 @@
>>> #define IPC_WWBUF_SIZE 20 /* IPC Write buffer Size */
>>> #define IPC_RWBUF_SIZE 20 /* IPC Read buffer Size */
>>> #define IPC_IOC 0x100 /* IPC command register
>>> IOC bit */
>>> +#define IPC_CMD_SIZE 16
>>> +#define IPC_CMD_SUBCMD 12
>>> +#define IPC_RWBUF_SIZE_DWORD 5
>>> +#define IPC_WWBUF_SIZE_DWORD 5
>>> +
>>> #define PCI_DEVICE_ID_LINCROFT 0x082a
>>> #define PCI_DEVICE_ID_PENWELL 0x080e
>>> @@ -93,140 +119,49 @@ static struct intel_scu_ipc_pdata_t
>>> intel_scu_ipc_tangier_pdata = {
>>> struct intel_scu_ipc_dev {
>>> struct device *dev;
>>> + struct intel_ipc_dev *ipc_dev;
>>> void __iomem *ipc_base;
>>> void __iomem *i2c_base;
>>> - struct completion cmd_complete;
>>> + struct regmap *ipc_regs;
>>> + struct regmap *i2c_regs;
>>> u8 irq_mode;
>>> };
>>> -static struct intel_scu_ipc_dev ipcdev; /* Only one for now */
>>> +static struct regmap_config ipc_regmap_config = {
>>> + .reg_bits = 32,
>>> + .reg_stride = 4,
>>> + .val_bits = 32,
>>> +};
>>> -/*
>>> - * IPC Read Buffer (Read Only):
>>> - * 16 byte buffer for receiving data from SCU, if IPC command
>>> - * processing results in response data
>>> - */
>>> -#define IPC_READ_BUFFER 0x90
>>> +static struct regmap_config i2c_regmap_config = {
>>> + .reg_bits = 32,
>>> + .reg_stride = 4,
>>> + .val_bits = 32,
>>> + .fast_io = true,
>>> +};
>>> +
>>> +static struct intel_scu_ipc_dev ipcdev; /* Only one for now */
>>> #define IPC_I2C_CNTRL_ADDR 0
>>> #define I2C_DATA_ADDR 0x04
>>> -static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple
>>> call to SCU */
>>> -
>>> -/*
>>> - * Send ipc command
>>> - * Command Register (Write Only):
>>> - * A write to this register results in an interrupt to the SCU core
>>> processor
>>> - * Format:
>>> - * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)|
>>> - */
>>> -static inline void ipc_command(struct intel_scu_ipc_dev *scu, u32 cmd)
>>> -{
>>> - if (scu->irq_mode) {
>>> - reinit_completion(&scu->cmd_complete);
>>> - writel(cmd | IPC_IOC, scu->ipc_base);
>>> - }
>>> - writel(cmd, scu->ipc_base);
>>> -}
>>> -
>>> -/*
>>> - * Write ipc data
>>> - * IPC Write Buffer (Write Only):
>>> - * 16-byte buffer for sending data associated with IPC command to
>>> - * SCU. Size of the data is specified in the IPC_COMMAND_REG register
>>> - */
>>> -static inline void ipc_data_writel(struct intel_scu_ipc_dev *scu,
>>> u32 data, u32 offset)
>>> -{
>>> - writel(data, scu->ipc_base + 0x80 + offset);
>>> -}
>>> -
>>> -/*
>>> - * Status Register (Read Only):
>>> - * Driver will read this register to get the ready/busy status of
>>> the IPC
>>> - * block and error status of the IPC command that was just
>>> processed by SCU
>>> - * Format:
>>> - * |rfu3(8)|error code(8)|initiator id(8)|cmd
>>> id(4)|rfu1(2)|error(1)|busy(1)|
>>> - */
>>> -static inline u8 ipc_read_status(struct intel_scu_ipc_dev *scu)
>>> -{
>>> - return __raw_readl(scu->ipc_base + 0x04);
>>> -}
>>> -
>>> -/* Read ipc byte data */
>>> -static inline u8 ipc_data_readb(struct intel_scu_ipc_dev *scu, u32
>>> offset)
>>> -{
>>> - return readb(scu->ipc_base + IPC_READ_BUFFER + offset);
>>> -}
>>> -
>>> -/* Read ipc u32 data */
>>> -static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32
>>> offset)
>>> -{
>>> - return readl(scu->ipc_base + IPC_READ_BUFFER + offset);
>>> -}
>>> -
>>> -/* Wait till scu status is busy */
>>> -static inline int busy_loop(struct intel_scu_ipc_dev *scu)
>>> -{
>>> - u32 status = ipc_read_status(scu);
>>> - u32 loop_count = 100000;
>>> -
>>> - /* break if scu doesn't reset busy bit after huge retry */
>>> - while ((status & BIT(0)) && --loop_count) {
>>> - udelay(1); /* scu processing time is in few u secods */
>>> - status = ipc_read_status(scu);
>>> - }
>>> -
>>> - if (status & BIT(0)) {
>>> - dev_err(scu->dev, "IPC timed out");
>>> - return -ETIMEDOUT;
>>> - }
>>> -
>>> - if (status & BIT(1))
>>> - return -EIO;
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -/* Wait till ipc ioc interrupt is received or timeout in 3 HZ */
>>> -static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev
>>> *scu)
>>> -{
>>> - int status;
>>> -
>>> - if (!wait_for_completion_timeout(&scu->cmd_complete, 3 * HZ)) {
>>> - dev_err(scu->dev, "IPC timed out\n");
>>> - return -ETIMEDOUT;
>>> - }
>>> -
>>> - status = ipc_read_status(scu);
>>> - if (status & BIT(1))
>>> - return -EIO;
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
>>> -{
>>> - return scu->irq_mode ? ipc_wait_for_interrupt(scu) :
>>> busy_loop(scu);
>>> -}
>>> -
>>> /* Read/Write power control(PMIC in Langwell, MSIC in PenWell)
>>> registers */
>>> static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op,
>>> u32 id)
>>> {
>>> struct intel_scu_ipc_dev *scu = &ipcdev;
>>> int nc;
>>> u32 offset = 0;
>>> - int err;
>>> + int err = -EIO;
>>> u8 cbuf[IPC_WWBUF_SIZE];
>>> u32 *wbuf = (u32 *)&cbuf;
>>> + u32 cmd[SCU_PARAM_LEN] = {0};
>>> + /* max rbuf size is 20 bytes */
>>> + u8 rbuf[IPC_RWBUF_SIZE] = {0};
>>> + u32 rbuflen = DIV_ROUND_UP(count, 4);
>>> memset(cbuf, 0, sizeof(cbuf));
>>> - mutex_lock(&ipclock);
>>> -
>>> - if (scu->dev == NULL) {
>>> - mutex_unlock(&ipclock);
>>> - return -ENODEV;
>>> - }
>>> + scu_cmd_init(cmd, op, id);
>>> for (nc = 0; nc < count; nc++, offset += 2) {
>>> cbuf[offset] = addr[nc];
>>> @@ -234,30 +169,30 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data,
>>> u32 count, u32 op, u32 id)
>>> }
>>> if (id == IPC_CMD_PCNTRL_R) {
>>> - for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
>>> - ipc_data_writel(scu, wbuf[nc], offset);
>>> - ipc_command(scu, (count * 2) << 16 | id << 12 | 0 << 8 | op);
>>> + err = ipc_dev_raw_cmd(scu->ipc_dev, cmd, SCU_PARAM_LEN,
>>> + (u8 *)wbuf, count * 2, (u32 *)rbuf,
>>> + IPC_RWBUF_SIZE_DWORD, 0, 0);
>>> } else if (id == IPC_CMD_PCNTRL_W) {
>>> for (nc = 0; nc < count; nc++, offset += 1)
>>> cbuf[offset] = data[nc];
>>> - for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
>>> - ipc_data_writel(scu, wbuf[nc], offset);
>>> - ipc_command(scu, (count * 3) << 16 | id << 12 | 0 << 8 | op);
>>> + err = ipc_dev_raw_cmd(scu->ipc_dev, cmd, SCU_PARAM_LEN,
>>> + (u8 *)wbuf, count * 3, NULL, 0, 0, 0);
>>> +
>>> } else if (id == IPC_CMD_PCNTRL_M) {
>>> cbuf[offset] = data[0];
>>> cbuf[offset + 1] = data[1];
>>> - ipc_data_writel(scu, wbuf[0], 0); /* Write wbuff */
>>> - ipc_command(scu, 4 << 16 | id << 12 | 0 << 8 | op);
>>> + err = ipc_dev_raw_cmd(scu->ipc_dev, cmd, SCU_PARAM_LEN,
>>> + (u8 *)wbuf, 4, NULL, 0, 0, 0);
>>> }
>>> - err = intel_scu_ipc_check_status(scu);
>>> if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
>>> /* Workaround: values are read as 0 without memcpy_fromio */
>>> memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
>>> - for (nc = 0; nc < count; nc++)
>>> - data[nc] = ipc_data_readb(scu, nc);
>>> + regmap_bulk_read(scu->ipc_regs, IPC_DEV_SCU_RBUF_OFFSET,
>>> + rbuf, rbuflen);
>>> + memcpy(data, rbuf, count);
>>> }
>>> - mutex_unlock(&ipclock);
>>> +
>>> return err;
>>> }
>>> @@ -422,138 +357,6 @@ int intel_scu_ipc_update_register(u16 addr,
>>> u8 bits, u8 mask)
>>> }
>>> EXPORT_SYMBOL(intel_scu_ipc_update_register);
>>> -/**
>>> - * intel_scu_ipc_simple_command - send a simple command
>>> - * @cmd: command
>>> - * @sub: sub type
>>> - *
>>> - * Issue a simple command to the SCU. Do not use this interface if
>>> - * you must then access data as any data values may be overwritten
>>> - * by another SCU access by the time this function returns.
>>> - *
>>> - * This function may sleep. Locking for SCU accesses is handled for
>>> - * the caller.
>>> - */
>>> -int intel_scu_ipc_simple_command(int cmd, int sub)
>>> -{
>>> - struct intel_scu_ipc_dev *scu = &ipcdev;
>>> - int err;
>>> -
>>> - mutex_lock(&ipclock);
>>> - if (scu->dev == NULL) {
>>> - mutex_unlock(&ipclock);
>>> - return -ENODEV;
>>> - }
>>> - ipc_command(scu, sub << 12 | cmd);
>>> - err = intel_scu_ipc_check_status(scu);
>>> - mutex_unlock(&ipclock);
>>> - return err;
>>> -}
>>> -EXPORT_SYMBOL(intel_scu_ipc_simple_command);
>>> -
>>> -/**
>>> - * intel_scu_ipc_command - command with data
>>> - * @cmd: command
>>> - * @sub: sub type
>>> - * @in: input data
>>> - * @inlen: input length in dwords
>>> - * @out: output data
>>> - * @outlein: output length in dwords
>>> - *
>>> - * Issue a command to the SCU which involves data transfers. Do the
>>> - * data copies under the lock but leave it for the caller to
>>> interpret
>>> - */
>>> -int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
>>> - u32 *out, int outlen)
>>> -{
>>> - struct intel_scu_ipc_dev *scu = &ipcdev;
>>> - int i, err;
>>> -
>>> - mutex_lock(&ipclock);
>>> - if (scu->dev == NULL) {
>>> - mutex_unlock(&ipclock);
>>> - return -ENODEV;
>>> - }
>>> -
>>> - for (i = 0; i < inlen; i++)
>>> - ipc_data_writel(scu, *in++, 4 * i);
>>> -
>>> - ipc_command(scu, (inlen << 16) | (sub << 12) | cmd);
>>> - err = intel_scu_ipc_check_status(scu);
>>> -
>>> - if (!err) {
>>> - for (i = 0; i < outlen; i++)
>>> - *out++ = ipc_data_readl(scu, 4 * i);
>>> - }
>>> -
>>> - mutex_unlock(&ipclock);
>>> - return err;
>>> -}
>>> -EXPORT_SYMBOL(intel_scu_ipc_command);
>>> -
>>> -#define IPC_SPTR 0x08
>>> -#define IPC_DPTR 0x0C
>>> -
>>> -/**
>>> - * intel_scu_ipc_raw_command() - IPC command with data and pointers
>>> - * @cmd: IPC command code.
>>> - * @sub: IPC command sub type.
>>> - * @in: input data of this IPC command.
>>> - * @inlen: input data length in dwords.
>>> - * @out: output data of this IPC command.
>>> - * @outlen: output data length in dwords.
>>> - * @sptr: data writing to SPTR register.
>>> - * @dptr: data writing to DPTR register.
>>> - *
>>> - * Send an IPC command to SCU with input/output data and
>>> source/dest pointers.
>>> - *
>>> - * Return: an IPC error code or 0 on success.
>>> - */
>>> -int intel_scu_ipc_raw_command(int cmd, int sub, u8 *in, int inlen,
>>> - u32 *out, int outlen, u32 dptr, u32 sptr)
>>> -{
>>> - struct intel_scu_ipc_dev *scu = &ipcdev;
>>> - int inbuflen = DIV_ROUND_UP(inlen, 4);
>>> - u32 inbuf[4];
>>> - int i, err;
>>> -
>>> - /* Up to 16 bytes */
>>> - if (inbuflen > 4)
>>> - return -EINVAL;
>>> -
>>> - mutex_lock(&ipclock);
>>> - if (scu->dev == NULL) {
>>> - mutex_unlock(&ipclock);
>>> - return -ENODEV;
>>> - }
>>> -
>>> - writel(dptr, scu->ipc_base + IPC_DPTR);
>>> - writel(sptr, scu->ipc_base + IPC_SPTR);
>>> -
>>> - /*
>>> - * SRAM controller doesn't support 8-bit writes, it only
>>> - * supports 32-bit writes, so we have to copy input data into
>>> - * the temporary buffer, and SCU FW will use the inlen to
>>> - * determine the actual input data length in the temporary
>>> - * buffer.
>>> - */
>>> - memcpy(inbuf, in, inlen);
>>> -
>>> - for (i = 0; i < inbuflen; i++)
>>> - ipc_data_writel(scu, inbuf[i], 4 * i);
>>> -
>>> - ipc_command(scu, (inlen << 16) | (sub << 12) | cmd);
>>> - err = intel_scu_ipc_check_status(scu);
>>> - if (!err) {
>>> - for (i = 0; i < outlen; i++)
>>> - *out++ = ipc_data_readl(scu, 4 * i);
>>> - }
>>> -
>>> - mutex_unlock(&ipclock);
>>> - return err;
>>> -}
>>> -EXPORT_SYMBOL_GPL(intel_scu_ipc_raw_command);
>>> -
>>> /* I2C commands */
>>> #define IPC_I2C_WRITE 1 /* I2C Write command */
>>> #define IPC_I2C_READ 2 /* I2C Read command */
>>> @@ -575,48 +378,143 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
>>> struct intel_scu_ipc_dev *scu = &ipcdev;
>>> u32 cmd = 0;
>>> - mutex_lock(&ipclock);
>>> - if (scu->dev == NULL) {
>>> - mutex_unlock(&ipclock);
>>> - return -ENODEV;
>>> - }
>>> cmd = (addr >> 24) & 0xFF;
>>> if (cmd == IPC_I2C_READ) {
>>> - writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
>>> + regmap_write(scu->i2c_regs, IPC_I2C_CNTRL_ADDR, addr);
>>> /* Write not getting updated without delay */
>>> mdelay(1);
>>> - *data = readl(scu->i2c_base + I2C_DATA_ADDR);
>>> + regmap_read(scu->i2c_regs, I2C_DATA_ADDR, data);
>>> } else if (cmd == IPC_I2C_WRITE) {
>>> - writel(*data, scu->i2c_base + I2C_DATA_ADDR);
>>> + regmap_write(scu->i2c_regs, I2C_DATA_ADDR, *data);
>>> mdelay(1);
>>> - writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
>>> + regmap_write(scu->i2c_regs, IPC_I2C_CNTRL_ADDR, addr);
>>> } else {
>>> dev_err(scu->dev,
>>> "intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
>>> - mutex_unlock(&ipclock);
>>> return -EIO;
>>> }
>>> - mutex_unlock(&ipclock);
>>> return 0;
>>> }
>>> EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);
>>> -/*
>>> - * Interrupt handler gets called when ioc bit of IPC_COMMAND_REG
>>> set to 1
>>> - * When ioc bit is set to 1, caller api must wait for interrupt
>>> handler called
>>> - * which in turn unlocks the caller api. Currently this is not used
>>> - *
>>> - * This is edge triggered so we need take no action to clear anything
>>> - */
>>> -static irqreturn_t ioc(int irq, void *dev_id)
>>> +static int pre_simple_cmd_fn(u32 *cmd_list, u32 cmdlen)
>>> {
>>> - struct intel_scu_ipc_dev *scu = dev_id;
>>> + if (!cmd_list || cmdlen != SCU_PARAM_LEN)
>>> + return -EINVAL;
>>> - if (scu->irq_mode)
>>> - complete(&scu->cmd_complete);
>>> + cmd_list[0] |= (cmd_list[1] << IPC_CMD_SUBCMD);
>>> - return IRQ_HANDLED;
>>> + return 0;
>>> +}
>>> +
>>> +static int pre_cmd_fn(u32 *cmd_list, u32 cmdlen, u32 *in, u32 inlen,
>>> + u32 *out, u32 outlen)
>>> +{
>>> + int ret;
>>> +
>>> + if (inlen > IPC_WWBUF_SIZE_DWORD || outlen > IPC_RWBUF_SIZE_DWORD)
>>> + return -EINVAL;
>>> +
>>> + ret = pre_simple_cmd_fn(cmd_list, cmdlen);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + cmd_list[0] |= (inlen << IPC_CMD_SIZE);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int pre_raw_cmd_fn(u32 *cmd_list, u32 cmdlen, u8 *in, u32
>>> inlen,
>>> + u32 *out, u32 outlen, u32 dptr, u32 sptr)
>>> +{
>>> + int ret;
>>> +
>>> + if (inlen > IPC_WWBUF_SIZE || outlen > IPC_RWBUF_SIZE_DWORD)
>>> + return -EINVAL;
>>> +
>>> + ret = pre_simple_cmd_fn(cmd_list, cmdlen);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + cmd_list[0] |= (inlen << IPC_CMD_SIZE);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int scu_ipc_err_code(int status)
>>> +{
>>> + if (status & IPC_DEV_SCU_CMD_STATUS_ERR)
>>> + return (status & IPC_DEV_SCU_CMD_STATUS_ERR_MASK);
>>> + else
>>> + return 0;
>>> +}
>>> +
>>> +static int scu_ipc_busy_check(int status)
>>> +{
>>> + return status | IPC_DEV_SCU_CMD_STATUS_BUSY;
>>> +}
>>> +
>>> +static u32 scu_ipc_enable_msi(u32 cmd)
>>> +{
>>> + return cmd | IPC_DEV_SCU_CMD_MSI;
>>> +}
>>> +
>>> +static struct intel_ipc_dev *intel_scu_ipc_dev_create(
>>> + struct device *dev,
>>> + void __iomem *base,
>>> + int irq)
>>> +{
>>> + struct intel_ipc_dev_ops *ops;
>>> + struct intel_ipc_dev_cfg *cfg;
>>> + struct regmap *ipc_regs;
>>> + struct intel_scu_ipc_dev *scu = dev_get_drvdata(dev);
>>> +
>>> + cfg = devm_kzalloc(dev, sizeof(*cfg), GFP_KERNEL);
>>> + if (!cfg)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + ops = devm_kzalloc(dev, sizeof(*ops), GFP_KERNEL);
>>> + if (!ops)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + ipc_regs = devm_regmap_init_mmio_clk(dev, NULL, base,
>>> + &ipc_regmap_config);
>>> + if (IS_ERR(ipc_regs)) {
>>> + dev_err(dev, "ipc_regs regmap init failed\n");
>>> + return ERR_CAST(ipc_regs);;
>>> + }
>>> +
>>> + scu->ipc_regs = ipc_regs;
>>> +
>>> + /* set IPC dev ops */
>>> + ops->to_err_code = scu_ipc_err_code;
>>> + ops->busy_check = scu_ipc_busy_check;
>>> + ops->enable_msi = scu_ipc_enable_msi;
>>> + ops->pre_cmd_fn = pre_cmd_fn;
>>> + ops->pre_raw_cmd_fn = pre_raw_cmd_fn;
>>> + ops->pre_simple_cmd_fn = pre_simple_cmd_fn;
>>> +
>>> + /* set cfg options */
>>> + if (scu->irq_mode)
>>> + cfg->mode = IPC_DEV_MODE_IRQ;
>>> + else
>>> + cfg->mode = IPC_DEV_MODE_POLLING;
>>> +
>>> + cfg->chan_type = IPC_CHANNEL_IA_SCU;
>>> + cfg->irq = irq;
>>> + cfg->use_msi = true;
>>> + cfg->support_sptr = true;
>>> + cfg->support_dptr = true;
>>> + cfg->cmd_regs = ipc_regs;
>>> + cfg->data_regs = ipc_regs;
>>> + cfg->wrbuf_reg = IPC_DEV_SCU_WRBUF_OFFSET;
>>> + cfg->rbuf_reg = IPC_DEV_SCU_RBUF_OFFSET;
>>> + cfg->sptr_reg = IPC_DEV_SCU_SPTR_OFFSET;
>>> + cfg->dptr_reg = IPC_DEV_SCU_DPTR_OFFSET;
>>> + cfg->status_reg = IPC_DEV_SCU_STATUS_OFFSET;
>>> +
>>> + return devm_intel_ipc_dev_create(dev, INTEL_SCU_IPC_DEV, cfg,
>>> ops);
>>> }
>>> /**
>>> @@ -650,25 +548,34 @@ static int ipc_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> if (err)
>>> return err;
>>> - init_completion(&scu->cmd_complete);
>>> -
>>> scu->ipc_base = pcim_iomap_table(pdev)[0];
>>> - scu->i2c_base = ioremap_nocache(pdata->i2c_base,
>>> pdata->i2c_len);
>>> + scu->i2c_base = devm_ioremap_nocache(&pdev->dev, pdata->i2c_base,
>>> + pdata->i2c_len);
>>> if (!scu->i2c_base)
>>> return -ENOMEM;
>>> - err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0,
>>> "intel_scu_ipc",
>>> - scu);
>>> - if (err)
>>> - return err;
>>> + pci_set_drvdata(pdev, scu);
>>> +
>>> + scu->i2c_regs = devm_regmap_init_mmio_clk(&pdev->dev, NULL,
>>> + scu->i2c_base, &i2c_regmap_config);
>>> + if (IS_ERR(scu->i2c_regs)) {
>>> + dev_err(&pdev->dev, "i2c_regs regmap init failed\n");
>>> + return PTR_ERR(scu->i2c_regs);;
>>> + }
>>> +
>>> + scu->ipc_dev = intel_scu_ipc_dev_create(&pdev->dev, scu->ipc_base,
>>> + pdev->irq);
>>> + if (IS_ERR(scu->ipc_dev)) {
>>> + dev_err(&pdev->dev, "Failed to create SCU IPC device\n");
>>> + return PTR_ERR(scu->ipc_dev);
>>> + }
>>> /* Assign device at last */
>>> scu->dev = &pdev->dev;
>>> intel_scu_devices_create();
>>> - pci_set_drvdata(pdev, scu);
>>> return 0;
>>> }
>>> diff --git a/drivers/rtc/rtc-mrst.c b/drivers/rtc/rtc-mrst.c
>>> index 7334c44..a2d87e8 100644
>>> --- a/drivers/rtc/rtc-mrst.c
>>> +++ b/drivers/rtc/rtc-mrst.c
>>> @@ -36,6 +36,7 @@
>>> #include <linux/module.h>
>>> #include <linux/init.h>
>>> #include <linux/sfi.h>
>>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>> #include <asm/intel_scu_ipc.h>
>>> #include <asm/intel-mid.h>
>>> @@ -46,6 +47,7 @@ struct mrst_rtc {
>>> struct device *dev;
>>> int irq;
>>> struct resource *iomem;
>>> + struct intel_ipc_dev *ipc_dev;
>>> u8 enabled_wake;
>>> u8 suspend_ctrl;
>>> @@ -110,10 +112,11 @@ static int mrst_read_time(struct device *dev,
>>> struct rtc_time *time)
>>> static int mrst_set_time(struct device *dev, struct rtc_time *time)
>>> {
>>> - int ret;
>>> unsigned long flags;
>>> unsigned char mon, day, hrs, min, sec;
>>> unsigned int yrs;
>>> + struct mrst_rtc *mrst = dev_get_drvdata(dev);
>>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_VRTC, IPC_CMD_VRTC_SETTIME};
>>> yrs = time->tm_year;
>>> mon = time->tm_mon + 1; /* tm_mon starts at zero */
>>> @@ -137,8 +140,7 @@ static int mrst_set_time(struct device *dev,
>>> struct rtc_time *time)
>>> spin_unlock_irqrestore(&rtc_lock, flags);
>>> - ret = intel_scu_ipc_simple_command(IPCMSG_VRTC,
>>> IPC_CMD_VRTC_SETTIME);
>>> - return ret;
>>> + return ipc_dev_simple_cmd(mrst->ipc_dev, cmds, SCU_PARAM_LEN);
>>> }
>>> static int mrst_read_alarm(struct device *dev, struct rtc_wkalrm
>>> *t)
>>> @@ -210,6 +212,7 @@ static int mrst_set_alarm(struct device *dev,
>>> struct rtc_wkalrm *t)
>>> struct mrst_rtc *mrst = dev_get_drvdata(dev);
>>> unsigned char hrs, min, sec;
>>> int ret = 0;
>>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_VRTC, IPC_CMD_VRTC_SETALARM};
>>> if (!mrst->irq)
>>> return -EIO;
>>> @@ -229,7 +232,7 @@ static int mrst_set_alarm(struct device *dev,
>>> struct rtc_wkalrm *t)
>>> spin_unlock_irq(&rtc_lock);
>>> - ret = intel_scu_ipc_simple_command(IPCMSG_VRTC,
>>> IPC_CMD_VRTC_SETALARM);
>>> + ret = ipc_dev_simple_cmd(mrst->ipc_dev, cmds, SCU_PARAM_LEN);
>>> if (ret)
>>> return ret;
>>> @@ -329,6 +332,10 @@ static int vrtc_mrst_do_probe(struct device
>>> *dev, struct resource *iomem,
>>> if (!iomem)
>>> return -ENODEV;
>>> + mrst_rtc.ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>>> + if (IS_ERR_OR_NULL(mrst_rtc.ipc_dev))
>>> + return PTR_ERR(mrst_rtc.ipc_dev);
>>> +
>>> iomem = request_mem_region(iomem->start, resource_size(iomem),
>>> driver_name);
>>> if (!iomem) {
>>> diff --git a/drivers/watchdog/intel-mid_wdt.c
>>> b/drivers/watchdog/intel-mid_wdt.c
>>> index 72c108a..a73559b 100644
>>> --- a/drivers/watchdog/intel-mid_wdt.c
>>> +++ b/drivers/watchdog/intel-mid_wdt.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/watchdog.h>
>>> #include <linux/platform_data/intel-mid_wdt.h>
>>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>> #include <asm/intel_scu_ipc.h>
>>> #include <asm/intel-mid.h>
>>> @@ -29,6 +30,8 @@
>>> #define MID_WDT_TIMEOUT_MAX 170
>>> #define MID_WDT_DEFAULT_TIMEOUT 90
>>> +static struct intel_ipc_dev *scu_ipc_dev;
>>> +
>>> /* SCU watchdog messages */
>>> enum {
>>> SCU_WATCHDOG_START = 0,
>>> @@ -38,7 +41,10 @@ enum {
>>> static inline int wdt_command(int sub, u32 *in, int inlen)
>>> {
>>> - return intel_scu_ipc_command(IPC_WATCHDOG, sub, in, inlen,
>>> NULL, 0);
>>> + u32 cmds[SCU_PARAM_LEN] = {IPC_WATCHDOG, sub};
>>> +
>>> + return ipc_dev_cmd(scu_ipc_dev, cmds, SCU_PARAM_LEN, in,
>>> + inlen, NULL, 0);
>>> }
>>> static int wdt_start(struct watchdog_device *wd)
>>> @@ -129,6 +135,10 @@ static int mid_wdt_probe(struct platform_device
>>> *pdev)
>>> if (!wdt_dev)
>>> return -ENOMEM;
>>> + scu_ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>>> + if (IS_ERR_OR_NULL(scu_ipc_dev))
>>> + return PTR_ERR(scu_ipc_dev);
>>> +
>>> wdt_dev->info = &mid_wdt_info;
>>> wdt_dev->ops = &mid_wdt_ops;
>>> wdt_dev->min_timeout = MID_WDT_TIMEOUT_MIN;
>>> diff --git a/drivers/watchdog/intel_scu_watchdog.c
>>> b/drivers/watchdog/intel_scu_watchdog.c
>>> index 0caab62..9457c7a 100644
>>> --- a/drivers/watchdog/intel_scu_watchdog.c
>>> +++ b/drivers/watchdog/intel_scu_watchdog.c
>>> @@ -49,6 +49,7 @@
>>> #include <asm/intel_scu_ipc.h>
>>> #include <asm/apb_timer.h>
>>> #include <asm/intel-mid.h>
>>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>> #include "intel_scu_watchdog.h"
>>> @@ -94,6 +95,8 @@ MODULE_PARM_DESC(force_boot,
>>> static struct intel_scu_watchdog_dev watchdog_device;
>>> +static struct intel_ipc_dev *scu_ipc_dev;
>>> +
>>> /* Forces restart, if force_reboot is set */
>>> static void watchdog_fire(void)
>>> {
>>> @@ -128,18 +131,14 @@ static int watchdog_set_ipc(int
>>> soft_threshold, int threshold)
>>> u32 *ipc_wbuf;
>>> u8 cbuf[16] = { '\0' };
>>> int ipc_ret = 0;
>>> + u32 cmds[SCU_PARAM_LEN] = {IPC_SET_WATCHDOG_TIMER, 0};
>>> ipc_wbuf = (u32 *)&cbuf;
>>> ipc_wbuf[0] = soft_threshold;
>>> ipc_wbuf[1] = threshold;
>>> - ipc_ret = intel_scu_ipc_command(
>>> - IPC_SET_WATCHDOG_TIMER,
>>> - 0,
>>> - ipc_wbuf,
>>> - 2,
>>> - NULL,
>>> - 0);
>>> + ipc_ret = ipc_dev_cmd(scu_ipc_dev, cmds, SCU_PARAM_LEN, ipc_wbuf,
>>> + 2, NULL, 0);
>>> if (ipc_ret != 0)
>>> pr_err("Error setting SCU watchdog timer: %x\n", ipc_ret);
>>> @@ -460,6 +459,10 @@ static int __init intel_scu_watchdog_init(void)
>>> if (check_timer_margin(timer_margin))
>>> return -EINVAL;
>>> + scu_ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>>> + if (IS_ERR_OR_NULL(scu_ipc_dev))
>>> + return PTR_ERR(scu_ipc_dev);
>>> +
>>> watchdog_device.timer_tbl_ptr = sfi_get_mtmr(sfi_mtimer_num-1);
>>> if (watchdog_device.timer_tbl_ptr == NULL) {
>>> --
>>> 2.7.4
>>>
>>
>
>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [RFC v3 7/7] platform/x86: intel_scu_ipc: Use generic Intel IPC device calls
From: sathyanarayanan kuppuswamy @ 2017-10-04 0:32 UTC (permalink / raw)
To: Alexandre Belloni
Cc: a.zummo, x86, wim, mingo, qipeng.zha, hpa, dvhart, tglx,
lee.jones, andy, souvik.k.chakravarty, linux-rtc, linux-watchdog,
linux-kernel, platform-driver-x86, sathyaosid
In-Reply-To: <20170928125519.ol5suebaoqsiixm5@piout.net>
Hi Alexandre,
On 09/28/2017 05:55 AM, Alexandre Belloni wrote:
> On 04/09/2017 at 22:37:27 -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> Removed redundant IPC helper functions and refactored the driver to use
>> generic IPC device driver APIs.
>>
>> This patch also cleans-up SCU IPC user drivers to use APIs provided
>> by generic IPC driver.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> For the RTC part:
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Thanks for the review.
>
>
>> ---
>> arch/x86/include/asm/intel_scu_ipc.h | 23 +-
>> arch/x86/platform/intel-mid/intel-mid.c | 12 +-
>> drivers/platform/x86/Kconfig | 1 +
>> drivers/platform/x86/intel_scu_ipc.c | 483 +++++++++++++-------------------
>> drivers/rtc/rtc-mrst.c | 15 +-
>> drivers/watchdog/intel-mid_wdt.c | 12 +-
>> drivers/watchdog/intel_scu_watchdog.c | 17 +-
>> 7 files changed, 251 insertions(+), 312 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/intel_scu_ipc.h b/arch/x86/include/asm/intel_scu_ipc.h
>> index 81d3d87..5842534 100644
>> --- a/arch/x86/include/asm/intel_scu_ipc.h
>> +++ b/arch/x86/include/asm/intel_scu_ipc.h
>> @@ -14,9 +14,19 @@
>> #define IPCMSG_COLD_BOOT 0xF3
>>
>> #define IPCMSG_VRTC 0xFA /* Set vRTC device */
>> - /* Command id associated with message IPCMSG_VRTC */
>> - #define IPC_CMD_VRTC_SETTIME 1 /* Set time */
>> - #define IPC_CMD_VRTC_SETALARM 2 /* Set alarm */
>> +
>> +/* Command id associated with message IPCMSG_VRTC */
>> +#define IPC_CMD_VRTC_SETTIME 1 /* Set time */
>> +#define IPC_CMD_VRTC_SETALARM 2 /* Set alarm */
>> +
>> +#define INTEL_SCU_IPC_DEV "intel_scu_ipc"
>> +#define SCU_PARAM_LEN 2
>> +
>> +static inline void scu_cmd_init(u32 *cmd, u32 param1, u32 param2)
>> +{
>> + cmd[0] = param1;
>> + cmd[1] = param2;
>> +}
>>
>> /* Read single register */
>> int intel_scu_ipc_ioread8(u16 addr, u8 *data);
>> @@ -45,13 +55,6 @@ int intel_scu_ipc_writev(u16 *addr, u8 *data, int len);
>> /* Update single register based on the mask */
>> int intel_scu_ipc_update_register(u16 addr, u8 data, u8 mask);
>>
>> -/* Issue commands to the SCU with or without data */
>> -int intel_scu_ipc_simple_command(int cmd, int sub);
>> -int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
>> - u32 *out, int outlen);
>> -int intel_scu_ipc_raw_command(int cmd, int sub, u8 *in, int inlen,
>> - u32 *out, int outlen, u32 dptr, u32 sptr);
>> -
>> /* I2C control api */
>> int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data);
>>
>> diff --git a/arch/x86/platform/intel-mid/intel-mid.c b/arch/x86/platform/intel-mid/intel-mid.c
>> index 12a2725..27541e9 100644
>> --- a/arch/x86/platform/intel-mid/intel-mid.c
>> +++ b/arch/x86/platform/intel-mid/intel-mid.c
>> @@ -22,6 +22,7 @@
>> #include <linux/irq.h>
>> #include <linux/export.h>
>> #include <linux/notifier.h>
>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>
>> #include <asm/setup.h>
>> #include <asm/mpspec_def.h>
>> @@ -68,18 +69,23 @@ static void *(*get_intel_mid_ops[])(void) = INTEL_MID_OPS_INIT;
>> enum intel_mid_cpu_type __intel_mid_cpu_chip;
>> EXPORT_SYMBOL_GPL(__intel_mid_cpu_chip);
>>
>> +static struct intel_ipc_dev *ipc_dev;
>> +
>> static void intel_mid_power_off(void)
>> {
>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_COLD_OFF, 1};
>> /* Shut down South Complex via PWRMU */
>> intel_mid_pwr_power_off();
>>
>> /* Only for Tangier, the rest will ignore this command */
>> - intel_scu_ipc_simple_command(IPCMSG_COLD_OFF, 1);
>> + ipc_dev_simple_cmd(ipc_dev, cmds, SCU_PARAM_LEN);
>> };
>>
>> static void intel_mid_reboot(void)
>> {
>> - intel_scu_ipc_simple_command(IPCMSG_COLD_BOOT, 0);
>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_COLD_BOOT, 0};
>> +
>> + ipc_dev_simple_cmd(ipc_dev, cmds, SCU_PARAM_LEN);
>> }
>>
>> static unsigned long __init intel_mid_calibrate_tsc(void)
>> @@ -206,6 +212,8 @@ void __init x86_intel_mid_early_setup(void)
>> x86_init.mpparse.find_smp_config = x86_init_noop;
>> x86_init.mpparse.get_smp_config = x86_init_uint_noop;
>> set_bit(MP_BUS_ISA, mp_bus_not_pci);
>> +
>> + ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>> }
>>
>> /*
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 82479ca..e4e5822 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -850,6 +850,7 @@ config INTEL_VBTN
>> config INTEL_SCU_IPC
>> bool "Intel SCU IPC Support"
>> depends on X86_INTEL_MID
>> + select REGMAP_MMIO
>> default y
>> ---help---
>> IPC is used to bridge the communications between kernel and SCU on
>> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
>> index f7cf981..78013e4 100644
>> --- a/drivers/platform/x86/intel_scu_ipc.c
>> +++ b/drivers/platform/x86/intel_scu_ipc.c
>> @@ -24,6 +24,8 @@
>> #include <linux/pci.h>
>> #include <linux/interrupt.h>
>> #include <linux/sfi.h>
>> +#include <linux/regmap.h>
>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>> #include <asm/intel-mid.h>
>> #include <asm/intel_scu_ipc.h>
>>
>> @@ -39,6 +41,25 @@
>> #define IPC_CMD_PCNTRL_R 1 /* Register read */
>> #define IPC_CMD_PCNTRL_M 2 /* Register read-modify-write */
>>
>> +/* IPC dev register offsets */
>> +/*
>> + * IPC Read Buffer (Read Only):
>> + * 16 byte buffer for receiving data from SCU, if IPC command
>> + * processing results in response data
>> + */
>> +#define IPC_DEV_SCU_RBUF_OFFSET 0x90
>> +#define IPC_DEV_SCU_WRBUF_OFFSET 0x80
>> +#define IPC_DEV_SCU_SPTR_OFFSET 0x08
>> +#define IPC_DEV_SCU_DPTR_OFFSET 0x0C
>> +#define IPC_DEV_SCU_STATUS_OFFSET 0x04
>> +
>> +/* IPC dev commands */
>> +/* IPC command register IOC bit */
>> +#define IPC_DEV_SCU_CMD_MSI BIT(8)
>> +#define IPC_DEV_SCU_CMD_STATUS_ERR BIT(1)
>> +#define IPC_DEV_SCU_CMD_STATUS_ERR_MASK GENMASK(7, 0)
>> +#define IPC_DEV_SCU_CMD_STATUS_BUSY BIT(0)
>> +
>> /*
>> * IPC register summary
>> *
>> @@ -59,6 +80,11 @@
>> #define IPC_WWBUF_SIZE 20 /* IPC Write buffer Size */
>> #define IPC_RWBUF_SIZE 20 /* IPC Read buffer Size */
>> #define IPC_IOC 0x100 /* IPC command register IOC bit */
>> +#define IPC_CMD_SIZE 16
>> +#define IPC_CMD_SUBCMD 12
>> +#define IPC_RWBUF_SIZE_DWORD 5
>> +#define IPC_WWBUF_SIZE_DWORD 5
>> +
>>
>> #define PCI_DEVICE_ID_LINCROFT 0x082a
>> #define PCI_DEVICE_ID_PENWELL 0x080e
>> @@ -93,140 +119,49 @@ static struct intel_scu_ipc_pdata_t intel_scu_ipc_tangier_pdata = {
>>
>> struct intel_scu_ipc_dev {
>> struct device *dev;
>> + struct intel_ipc_dev *ipc_dev;
>> void __iomem *ipc_base;
>> void __iomem *i2c_base;
>> - struct completion cmd_complete;
>> + struct regmap *ipc_regs;
>> + struct regmap *i2c_regs;
>> u8 irq_mode;
>> };
>>
>> -static struct intel_scu_ipc_dev ipcdev; /* Only one for now */
>> +static struct regmap_config ipc_regmap_config = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> +};
>>
>> -/*
>> - * IPC Read Buffer (Read Only):
>> - * 16 byte buffer for receiving data from SCU, if IPC command
>> - * processing results in response data
>> - */
>> -#define IPC_READ_BUFFER 0x90
>> +static struct regmap_config i2c_regmap_config = {
>> + .reg_bits = 32,
>> + .reg_stride = 4,
>> + .val_bits = 32,
>> + .fast_io = true,
>> +};
>> +
>> +static struct intel_scu_ipc_dev ipcdev; /* Only one for now */
>>
>> #define IPC_I2C_CNTRL_ADDR 0
>> #define I2C_DATA_ADDR 0x04
>>
>> -static DEFINE_MUTEX(ipclock); /* lock used to prevent multiple call to SCU */
>> -
>> -/*
>> - * Send ipc command
>> - * Command Register (Write Only):
>> - * A write to this register results in an interrupt to the SCU core processor
>> - * Format:
>> - * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)|
>> - */
>> -static inline void ipc_command(struct intel_scu_ipc_dev *scu, u32 cmd)
>> -{
>> - if (scu->irq_mode) {
>> - reinit_completion(&scu->cmd_complete);
>> - writel(cmd | IPC_IOC, scu->ipc_base);
>> - }
>> - writel(cmd, scu->ipc_base);
>> -}
>> -
>> -/*
>> - * Write ipc data
>> - * IPC Write Buffer (Write Only):
>> - * 16-byte buffer for sending data associated with IPC command to
>> - * SCU. Size of the data is specified in the IPC_COMMAND_REG register
>> - */
>> -static inline void ipc_data_writel(struct intel_scu_ipc_dev *scu, u32 data, u32 offset)
>> -{
>> - writel(data, scu->ipc_base + 0x80 + offset);
>> -}
>> -
>> -/*
>> - * Status Register (Read Only):
>> - * Driver will read this register to get the ready/busy status of the IPC
>> - * block and error status of the IPC command that was just processed by SCU
>> - * Format:
>> - * |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)|
>> - */
>> -static inline u8 ipc_read_status(struct intel_scu_ipc_dev *scu)
>> -{
>> - return __raw_readl(scu->ipc_base + 0x04);
>> -}
>> -
>> -/* Read ipc byte data */
>> -static inline u8 ipc_data_readb(struct intel_scu_ipc_dev *scu, u32 offset)
>> -{
>> - return readb(scu->ipc_base + IPC_READ_BUFFER + offset);
>> -}
>> -
>> -/* Read ipc u32 data */
>> -static inline u32 ipc_data_readl(struct intel_scu_ipc_dev *scu, u32 offset)
>> -{
>> - return readl(scu->ipc_base + IPC_READ_BUFFER + offset);
>> -}
>> -
>> -/* Wait till scu status is busy */
>> -static inline int busy_loop(struct intel_scu_ipc_dev *scu)
>> -{
>> - u32 status = ipc_read_status(scu);
>> - u32 loop_count = 100000;
>> -
>> - /* break if scu doesn't reset busy bit after huge retry */
>> - while ((status & BIT(0)) && --loop_count) {
>> - udelay(1); /* scu processing time is in few u secods */
>> - status = ipc_read_status(scu);
>> - }
>> -
>> - if (status & BIT(0)) {
>> - dev_err(scu->dev, "IPC timed out");
>> - return -ETIMEDOUT;
>> - }
>> -
>> - if (status & BIT(1))
>> - return -EIO;
>> -
>> - return 0;
>> -}
>> -
>> -/* Wait till ipc ioc interrupt is received or timeout in 3 HZ */
>> -static inline int ipc_wait_for_interrupt(struct intel_scu_ipc_dev *scu)
>> -{
>> - int status;
>> -
>> - if (!wait_for_completion_timeout(&scu->cmd_complete, 3 * HZ)) {
>> - dev_err(scu->dev, "IPC timed out\n");
>> - return -ETIMEDOUT;
>> - }
>> -
>> - status = ipc_read_status(scu);
>> - if (status & BIT(1))
>> - return -EIO;
>> -
>> - return 0;
>> -}
>> -
>> -static int intel_scu_ipc_check_status(struct intel_scu_ipc_dev *scu)
>> -{
>> - return scu->irq_mode ? ipc_wait_for_interrupt(scu) : busy_loop(scu);
>> -}
>> -
>> /* Read/Write power control(PMIC in Langwell, MSIC in PenWell) registers */
>> static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
>> {
>> struct intel_scu_ipc_dev *scu = &ipcdev;
>> int nc;
>> u32 offset = 0;
>> - int err;
>> + int err = -EIO;
>> u8 cbuf[IPC_WWBUF_SIZE];
>> u32 *wbuf = (u32 *)&cbuf;
>> + u32 cmd[SCU_PARAM_LEN] = {0};
>> + /* max rbuf size is 20 bytes */
>> + u8 rbuf[IPC_RWBUF_SIZE] = {0};
>> + u32 rbuflen = DIV_ROUND_UP(count, 4);
>>
>> memset(cbuf, 0, sizeof(cbuf));
>>
>> - mutex_lock(&ipclock);
>> -
>> - if (scu->dev == NULL) {
>> - mutex_unlock(&ipclock);
>> - return -ENODEV;
>> - }
>> + scu_cmd_init(cmd, op, id);
>>
>> for (nc = 0; nc < count; nc++, offset += 2) {
>> cbuf[offset] = addr[nc];
>> @@ -234,30 +169,30 @@ static int pwr_reg_rdwr(u16 *addr, u8 *data, u32 count, u32 op, u32 id)
>> }
>>
>> if (id == IPC_CMD_PCNTRL_R) {
>> - for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
>> - ipc_data_writel(scu, wbuf[nc], offset);
>> - ipc_command(scu, (count * 2) << 16 | id << 12 | 0 << 8 | op);
>> + err = ipc_dev_raw_cmd(scu->ipc_dev, cmd, SCU_PARAM_LEN,
>> + (u8 *)wbuf, count * 2, (u32 *)rbuf,
>> + IPC_RWBUF_SIZE_DWORD, 0, 0);
>> } else if (id == IPC_CMD_PCNTRL_W) {
>> for (nc = 0; nc < count; nc++, offset += 1)
>> cbuf[offset] = data[nc];
>> - for (nc = 0, offset = 0; nc < count; nc++, offset += 4)
>> - ipc_data_writel(scu, wbuf[nc], offset);
>> - ipc_command(scu, (count * 3) << 16 | id << 12 | 0 << 8 | op);
>> + err = ipc_dev_raw_cmd(scu->ipc_dev, cmd, SCU_PARAM_LEN,
>> + (u8 *)wbuf, count * 3, NULL, 0, 0, 0);
>> +
>> } else if (id == IPC_CMD_PCNTRL_M) {
>> cbuf[offset] = data[0];
>> cbuf[offset + 1] = data[1];
>> - ipc_data_writel(scu, wbuf[0], 0); /* Write wbuff */
>> - ipc_command(scu, 4 << 16 | id << 12 | 0 << 8 | op);
>> + err = ipc_dev_raw_cmd(scu->ipc_dev, cmd, SCU_PARAM_LEN,
>> + (u8 *)wbuf, 4, NULL, 0, 0, 0);
>> }
>>
>> - err = intel_scu_ipc_check_status(scu);
>> if (!err && id == IPC_CMD_PCNTRL_R) { /* Read rbuf */
>> /* Workaround: values are read as 0 without memcpy_fromio */
>> memcpy_fromio(cbuf, scu->ipc_base + 0x90, 16);
>> - for (nc = 0; nc < count; nc++)
>> - data[nc] = ipc_data_readb(scu, nc);
>> + regmap_bulk_read(scu->ipc_regs, IPC_DEV_SCU_RBUF_OFFSET,
>> + rbuf, rbuflen);
>> + memcpy(data, rbuf, count);
>> }
>> - mutex_unlock(&ipclock);
>> +
>> return err;
>> }
>>
>> @@ -422,138 +357,6 @@ int intel_scu_ipc_update_register(u16 addr, u8 bits, u8 mask)
>> }
>> EXPORT_SYMBOL(intel_scu_ipc_update_register);
>>
>> -/**
>> - * intel_scu_ipc_simple_command - send a simple command
>> - * @cmd: command
>> - * @sub: sub type
>> - *
>> - * Issue a simple command to the SCU. Do not use this interface if
>> - * you must then access data as any data values may be overwritten
>> - * by another SCU access by the time this function returns.
>> - *
>> - * This function may sleep. Locking for SCU accesses is handled for
>> - * the caller.
>> - */
>> -int intel_scu_ipc_simple_command(int cmd, int sub)
>> -{
>> - struct intel_scu_ipc_dev *scu = &ipcdev;
>> - int err;
>> -
>> - mutex_lock(&ipclock);
>> - if (scu->dev == NULL) {
>> - mutex_unlock(&ipclock);
>> - return -ENODEV;
>> - }
>> - ipc_command(scu, sub << 12 | cmd);
>> - err = intel_scu_ipc_check_status(scu);
>> - mutex_unlock(&ipclock);
>> - return err;
>> -}
>> -EXPORT_SYMBOL(intel_scu_ipc_simple_command);
>> -
>> -/**
>> - * intel_scu_ipc_command - command with data
>> - * @cmd: command
>> - * @sub: sub type
>> - * @in: input data
>> - * @inlen: input length in dwords
>> - * @out: output data
>> - * @outlein: output length in dwords
>> - *
>> - * Issue a command to the SCU which involves data transfers. Do the
>> - * data copies under the lock but leave it for the caller to interpret
>> - */
>> -int intel_scu_ipc_command(int cmd, int sub, u32 *in, int inlen,
>> - u32 *out, int outlen)
>> -{
>> - struct intel_scu_ipc_dev *scu = &ipcdev;
>> - int i, err;
>> -
>> - mutex_lock(&ipclock);
>> - if (scu->dev == NULL) {
>> - mutex_unlock(&ipclock);
>> - return -ENODEV;
>> - }
>> -
>> - for (i = 0; i < inlen; i++)
>> - ipc_data_writel(scu, *in++, 4 * i);
>> -
>> - ipc_command(scu, (inlen << 16) | (sub << 12) | cmd);
>> - err = intel_scu_ipc_check_status(scu);
>> -
>> - if (!err) {
>> - for (i = 0; i < outlen; i++)
>> - *out++ = ipc_data_readl(scu, 4 * i);
>> - }
>> -
>> - mutex_unlock(&ipclock);
>> - return err;
>> -}
>> -EXPORT_SYMBOL(intel_scu_ipc_command);
>> -
>> -#define IPC_SPTR 0x08
>> -#define IPC_DPTR 0x0C
>> -
>> -/**
>> - * intel_scu_ipc_raw_command() - IPC command with data and pointers
>> - * @cmd: IPC command code.
>> - * @sub: IPC command sub type.
>> - * @in: input data of this IPC command.
>> - * @inlen: input data length in dwords.
>> - * @out: output data of this IPC command.
>> - * @outlen: output data length in dwords.
>> - * @sptr: data writing to SPTR register.
>> - * @dptr: data writing to DPTR register.
>> - *
>> - * Send an IPC command to SCU with input/output data and source/dest pointers.
>> - *
>> - * Return: an IPC error code or 0 on success.
>> - */
>> -int intel_scu_ipc_raw_command(int cmd, int sub, u8 *in, int inlen,
>> - u32 *out, int outlen, u32 dptr, u32 sptr)
>> -{
>> - struct intel_scu_ipc_dev *scu = &ipcdev;
>> - int inbuflen = DIV_ROUND_UP(inlen, 4);
>> - u32 inbuf[4];
>> - int i, err;
>> -
>> - /* Up to 16 bytes */
>> - if (inbuflen > 4)
>> - return -EINVAL;
>> -
>> - mutex_lock(&ipclock);
>> - if (scu->dev == NULL) {
>> - mutex_unlock(&ipclock);
>> - return -ENODEV;
>> - }
>> -
>> - writel(dptr, scu->ipc_base + IPC_DPTR);
>> - writel(sptr, scu->ipc_base + IPC_SPTR);
>> -
>> - /*
>> - * SRAM controller doesn't support 8-bit writes, it only
>> - * supports 32-bit writes, so we have to copy input data into
>> - * the temporary buffer, and SCU FW will use the inlen to
>> - * determine the actual input data length in the temporary
>> - * buffer.
>> - */
>> - memcpy(inbuf, in, inlen);
>> -
>> - for (i = 0; i < inbuflen; i++)
>> - ipc_data_writel(scu, inbuf[i], 4 * i);
>> -
>> - ipc_command(scu, (inlen << 16) | (sub << 12) | cmd);
>> - err = intel_scu_ipc_check_status(scu);
>> - if (!err) {
>> - for (i = 0; i < outlen; i++)
>> - *out++ = ipc_data_readl(scu, 4 * i);
>> - }
>> -
>> - mutex_unlock(&ipclock);
>> - return err;
>> -}
>> -EXPORT_SYMBOL_GPL(intel_scu_ipc_raw_command);
>> -
>> /* I2C commands */
>> #define IPC_I2C_WRITE 1 /* I2C Write command */
>> #define IPC_I2C_READ 2 /* I2C Read command */
>> @@ -575,48 +378,143 @@ int intel_scu_ipc_i2c_cntrl(u32 addr, u32 *data)
>> struct intel_scu_ipc_dev *scu = &ipcdev;
>> u32 cmd = 0;
>>
>> - mutex_lock(&ipclock);
>> - if (scu->dev == NULL) {
>> - mutex_unlock(&ipclock);
>> - return -ENODEV;
>> - }
>> cmd = (addr >> 24) & 0xFF;
>> if (cmd == IPC_I2C_READ) {
>> - writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
>> + regmap_write(scu->i2c_regs, IPC_I2C_CNTRL_ADDR, addr);
>> /* Write not getting updated without delay */
>> mdelay(1);
>> - *data = readl(scu->i2c_base + I2C_DATA_ADDR);
>> + regmap_read(scu->i2c_regs, I2C_DATA_ADDR, data);
>> } else if (cmd == IPC_I2C_WRITE) {
>> - writel(*data, scu->i2c_base + I2C_DATA_ADDR);
>> + regmap_write(scu->i2c_regs, I2C_DATA_ADDR, *data);
>> mdelay(1);
>> - writel(addr, scu->i2c_base + IPC_I2C_CNTRL_ADDR);
>> + regmap_write(scu->i2c_regs, IPC_I2C_CNTRL_ADDR, addr);
>> } else {
>> dev_err(scu->dev,
>> "intel_scu_ipc: I2C INVALID_CMD = 0x%x\n", cmd);
>>
>> - mutex_unlock(&ipclock);
>> return -EIO;
>> }
>> - mutex_unlock(&ipclock);
>> return 0;
>> }
>> EXPORT_SYMBOL(intel_scu_ipc_i2c_cntrl);
>>
>> -/*
>> - * Interrupt handler gets called when ioc bit of IPC_COMMAND_REG set to 1
>> - * When ioc bit is set to 1, caller api must wait for interrupt handler called
>> - * which in turn unlocks the caller api. Currently this is not used
>> - *
>> - * This is edge triggered so we need take no action to clear anything
>> - */
>> -static irqreturn_t ioc(int irq, void *dev_id)
>> +static int pre_simple_cmd_fn(u32 *cmd_list, u32 cmdlen)
>> {
>> - struct intel_scu_ipc_dev *scu = dev_id;
>> + if (!cmd_list || cmdlen != SCU_PARAM_LEN)
>> + return -EINVAL;
>>
>> - if (scu->irq_mode)
>> - complete(&scu->cmd_complete);
>> + cmd_list[0] |= (cmd_list[1] << IPC_CMD_SUBCMD);
>>
>> - return IRQ_HANDLED;
>> + return 0;
>> +}
>> +
>> +static int pre_cmd_fn(u32 *cmd_list, u32 cmdlen, u32 *in, u32 inlen,
>> + u32 *out, u32 outlen)
>> +{
>> + int ret;
>> +
>> + if (inlen > IPC_WWBUF_SIZE_DWORD || outlen > IPC_RWBUF_SIZE_DWORD)
>> + return -EINVAL;
>> +
>> + ret = pre_simple_cmd_fn(cmd_list, cmdlen);
>> + if (ret < 0)
>> + return ret;
>> +
>> + cmd_list[0] |= (inlen << IPC_CMD_SIZE);
>> +
>> + return 0;
>> +}
>> +
>> +static int pre_raw_cmd_fn(u32 *cmd_list, u32 cmdlen, u8 *in, u32 inlen,
>> + u32 *out, u32 outlen, u32 dptr, u32 sptr)
>> +{
>> + int ret;
>> +
>> + if (inlen > IPC_WWBUF_SIZE || outlen > IPC_RWBUF_SIZE_DWORD)
>> + return -EINVAL;
>> +
>> + ret = pre_simple_cmd_fn(cmd_list, cmdlen);
>> + if (ret < 0)
>> + return ret;
>> +
>> + cmd_list[0] |= (inlen << IPC_CMD_SIZE);
>> +
>> + return 0;
>> +}
>> +
>> +static int scu_ipc_err_code(int status)
>> +{
>> + if (status & IPC_DEV_SCU_CMD_STATUS_ERR)
>> + return (status & IPC_DEV_SCU_CMD_STATUS_ERR_MASK);
>> + else
>> + return 0;
>> +}
>> +
>> +static int scu_ipc_busy_check(int status)
>> +{
>> + return status | IPC_DEV_SCU_CMD_STATUS_BUSY;
>> +}
>> +
>> +static u32 scu_ipc_enable_msi(u32 cmd)
>> +{
>> + return cmd | IPC_DEV_SCU_CMD_MSI;
>> +}
>> +
>> +static struct intel_ipc_dev *intel_scu_ipc_dev_create(
>> + struct device *dev,
>> + void __iomem *base,
>> + int irq)
>> +{
>> + struct intel_ipc_dev_ops *ops;
>> + struct intel_ipc_dev_cfg *cfg;
>> + struct regmap *ipc_regs;
>> + struct intel_scu_ipc_dev *scu = dev_get_drvdata(dev);
>> +
>> + cfg = devm_kzalloc(dev, sizeof(*cfg), GFP_KERNEL);
>> + if (!cfg)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + ops = devm_kzalloc(dev, sizeof(*ops), GFP_KERNEL);
>> + if (!ops)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + ipc_regs = devm_regmap_init_mmio_clk(dev, NULL, base,
>> + &ipc_regmap_config);
>> + if (IS_ERR(ipc_regs)) {
>> + dev_err(dev, "ipc_regs regmap init failed\n");
>> + return ERR_CAST(ipc_regs);;
>> + }
>> +
>> + scu->ipc_regs = ipc_regs;
>> +
>> + /* set IPC dev ops */
>> + ops->to_err_code = scu_ipc_err_code;
>> + ops->busy_check = scu_ipc_busy_check;
>> + ops->enable_msi = scu_ipc_enable_msi;
>> + ops->pre_cmd_fn = pre_cmd_fn;
>> + ops->pre_raw_cmd_fn = pre_raw_cmd_fn;
>> + ops->pre_simple_cmd_fn = pre_simple_cmd_fn;
>> +
>> + /* set cfg options */
>> + if (scu->irq_mode)
>> + cfg->mode = IPC_DEV_MODE_IRQ;
>> + else
>> + cfg->mode = IPC_DEV_MODE_POLLING;
>> +
>> + cfg->chan_type = IPC_CHANNEL_IA_SCU;
>> + cfg->irq = irq;
>> + cfg->use_msi = true;
>> + cfg->support_sptr = true;
>> + cfg->support_dptr = true;
>> + cfg->cmd_regs = ipc_regs;
>> + cfg->data_regs = ipc_regs;
>> + cfg->wrbuf_reg = IPC_DEV_SCU_WRBUF_OFFSET;
>> + cfg->rbuf_reg = IPC_DEV_SCU_RBUF_OFFSET;
>> + cfg->sptr_reg = IPC_DEV_SCU_SPTR_OFFSET;
>> + cfg->dptr_reg = IPC_DEV_SCU_DPTR_OFFSET;
>> + cfg->status_reg = IPC_DEV_SCU_STATUS_OFFSET;
>> +
>> + return devm_intel_ipc_dev_create(dev, INTEL_SCU_IPC_DEV, cfg, ops);
>> }
>>
>> /**
>> @@ -650,25 +548,34 @@ static int ipc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> if (err)
>> return err;
>>
>> - init_completion(&scu->cmd_complete);
>> -
>> scu->ipc_base = pcim_iomap_table(pdev)[0];
>>
>> - scu->i2c_base = ioremap_nocache(pdata->i2c_base, pdata->i2c_len);
>> + scu->i2c_base = devm_ioremap_nocache(&pdev->dev, pdata->i2c_base,
>> + pdata->i2c_len);
>> if (!scu->i2c_base)
>> return -ENOMEM;
>>
>> - err = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_scu_ipc",
>> - scu);
>> - if (err)
>> - return err;
>> + pci_set_drvdata(pdev, scu);
>> +
>> + scu->i2c_regs = devm_regmap_init_mmio_clk(&pdev->dev, NULL,
>> + scu->i2c_base, &i2c_regmap_config);
>> + if (IS_ERR(scu->i2c_regs)) {
>> + dev_err(&pdev->dev, "i2c_regs regmap init failed\n");
>> + return PTR_ERR(scu->i2c_regs);;
>> + }
>> +
>> + scu->ipc_dev = intel_scu_ipc_dev_create(&pdev->dev, scu->ipc_base,
>> + pdev->irq);
>> + if (IS_ERR(scu->ipc_dev)) {
>> + dev_err(&pdev->dev, "Failed to create SCU IPC device\n");
>> + return PTR_ERR(scu->ipc_dev);
>> + }
>>
>> /* Assign device at last */
>> scu->dev = &pdev->dev;
>>
>> intel_scu_devices_create();
>>
>> - pci_set_drvdata(pdev, scu);
>> return 0;
>> }
>>
>> diff --git a/drivers/rtc/rtc-mrst.c b/drivers/rtc/rtc-mrst.c
>> index 7334c44..a2d87e8 100644
>> --- a/drivers/rtc/rtc-mrst.c
>> +++ b/drivers/rtc/rtc-mrst.c
>> @@ -36,6 +36,7 @@
>> #include <linux/module.h>
>> #include <linux/init.h>
>> #include <linux/sfi.h>
>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>
>> #include <asm/intel_scu_ipc.h>
>> #include <asm/intel-mid.h>
>> @@ -46,6 +47,7 @@ struct mrst_rtc {
>> struct device *dev;
>> int irq;
>> struct resource *iomem;
>> + struct intel_ipc_dev *ipc_dev;
>>
>> u8 enabled_wake;
>> u8 suspend_ctrl;
>> @@ -110,10 +112,11 @@ static int mrst_read_time(struct device *dev, struct rtc_time *time)
>>
>> static int mrst_set_time(struct device *dev, struct rtc_time *time)
>> {
>> - int ret;
>> unsigned long flags;
>> unsigned char mon, day, hrs, min, sec;
>> unsigned int yrs;
>> + struct mrst_rtc *mrst = dev_get_drvdata(dev);
>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_VRTC, IPC_CMD_VRTC_SETTIME};
>>
>> yrs = time->tm_year;
>> mon = time->tm_mon + 1; /* tm_mon starts at zero */
>> @@ -137,8 +140,7 @@ static int mrst_set_time(struct device *dev, struct rtc_time *time)
>>
>> spin_unlock_irqrestore(&rtc_lock, flags);
>>
>> - ret = intel_scu_ipc_simple_command(IPCMSG_VRTC, IPC_CMD_VRTC_SETTIME);
>> - return ret;
>> + return ipc_dev_simple_cmd(mrst->ipc_dev, cmds, SCU_PARAM_LEN);
>> }
>>
>> static int mrst_read_alarm(struct device *dev, struct rtc_wkalrm *t)
>> @@ -210,6 +212,7 @@ static int mrst_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>> struct mrst_rtc *mrst = dev_get_drvdata(dev);
>> unsigned char hrs, min, sec;
>> int ret = 0;
>> + u32 cmds[SCU_PARAM_LEN] = {IPCMSG_VRTC, IPC_CMD_VRTC_SETALARM};
>>
>> if (!mrst->irq)
>> return -EIO;
>> @@ -229,7 +232,7 @@ static int mrst_set_alarm(struct device *dev, struct rtc_wkalrm *t)
>>
>> spin_unlock_irq(&rtc_lock);
>>
>> - ret = intel_scu_ipc_simple_command(IPCMSG_VRTC, IPC_CMD_VRTC_SETALARM);
>> + ret = ipc_dev_simple_cmd(mrst->ipc_dev, cmds, SCU_PARAM_LEN);
>> if (ret)
>> return ret;
>>
>> @@ -329,6 +332,10 @@ static int vrtc_mrst_do_probe(struct device *dev, struct resource *iomem,
>> if (!iomem)
>> return -ENODEV;
>>
>> + mrst_rtc.ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>> + if (IS_ERR_OR_NULL(mrst_rtc.ipc_dev))
>> + return PTR_ERR(mrst_rtc.ipc_dev);
>> +
>> iomem = request_mem_region(iomem->start, resource_size(iomem),
>> driver_name);
>> if (!iomem) {
>> diff --git a/drivers/watchdog/intel-mid_wdt.c b/drivers/watchdog/intel-mid_wdt.c
>> index 72c108a..a73559b 100644
>> --- a/drivers/watchdog/intel-mid_wdt.c
>> +++ b/drivers/watchdog/intel-mid_wdt.c
>> @@ -18,6 +18,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/watchdog.h>
>> #include <linux/platform_data/intel-mid_wdt.h>
>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>
>> #include <asm/intel_scu_ipc.h>
>> #include <asm/intel-mid.h>
>> @@ -29,6 +30,8 @@
>> #define MID_WDT_TIMEOUT_MAX 170
>> #define MID_WDT_DEFAULT_TIMEOUT 90
>>
>> +static struct intel_ipc_dev *scu_ipc_dev;
>> +
>> /* SCU watchdog messages */
>> enum {
>> SCU_WATCHDOG_START = 0,
>> @@ -38,7 +41,10 @@ enum {
>>
>> static inline int wdt_command(int sub, u32 *in, int inlen)
>> {
>> - return intel_scu_ipc_command(IPC_WATCHDOG, sub, in, inlen, NULL, 0);
>> + u32 cmds[SCU_PARAM_LEN] = {IPC_WATCHDOG, sub};
>> +
>> + return ipc_dev_cmd(scu_ipc_dev, cmds, SCU_PARAM_LEN, in,
>> + inlen, NULL, 0);
>> }
>>
>> static int wdt_start(struct watchdog_device *wd)
>> @@ -129,6 +135,10 @@ static int mid_wdt_probe(struct platform_device *pdev)
>> if (!wdt_dev)
>> return -ENOMEM;
>>
>> + scu_ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>> + if (IS_ERR_OR_NULL(scu_ipc_dev))
>> + return PTR_ERR(scu_ipc_dev);
>> +
>> wdt_dev->info = &mid_wdt_info;
>> wdt_dev->ops = &mid_wdt_ops;
>> wdt_dev->min_timeout = MID_WDT_TIMEOUT_MIN;
>> diff --git a/drivers/watchdog/intel_scu_watchdog.c b/drivers/watchdog/intel_scu_watchdog.c
>> index 0caab62..9457c7a 100644
>> --- a/drivers/watchdog/intel_scu_watchdog.c
>> +++ b/drivers/watchdog/intel_scu_watchdog.c
>> @@ -49,6 +49,7 @@
>> #include <asm/intel_scu_ipc.h>
>> #include <asm/apb_timer.h>
>> #include <asm/intel-mid.h>
>> +#include <linux/platform_data/x86/intel_ipc_dev.h>
>>
>> #include "intel_scu_watchdog.h"
>>
>> @@ -94,6 +95,8 @@ MODULE_PARM_DESC(force_boot,
>>
>> static struct intel_scu_watchdog_dev watchdog_device;
>>
>> +static struct intel_ipc_dev *scu_ipc_dev;
>> +
>> /* Forces restart, if force_reboot is set */
>> static void watchdog_fire(void)
>> {
>> @@ -128,18 +131,14 @@ static int watchdog_set_ipc(int soft_threshold, int threshold)
>> u32 *ipc_wbuf;
>> u8 cbuf[16] = { '\0' };
>> int ipc_ret = 0;
>> + u32 cmds[SCU_PARAM_LEN] = {IPC_SET_WATCHDOG_TIMER, 0};
>>
>> ipc_wbuf = (u32 *)&cbuf;
>> ipc_wbuf[0] = soft_threshold;
>> ipc_wbuf[1] = threshold;
>>
>> - ipc_ret = intel_scu_ipc_command(
>> - IPC_SET_WATCHDOG_TIMER,
>> - 0,
>> - ipc_wbuf,
>> - 2,
>> - NULL,
>> - 0);
>> + ipc_ret = ipc_dev_cmd(scu_ipc_dev, cmds, SCU_PARAM_LEN, ipc_wbuf,
>> + 2, NULL, 0);
>>
>> if (ipc_ret != 0)
>> pr_err("Error setting SCU watchdog timer: %x\n", ipc_ret);
>> @@ -460,6 +459,10 @@ static int __init intel_scu_watchdog_init(void)
>> if (check_timer_margin(timer_margin))
>> return -EINVAL;
>>
>> + scu_ipc_dev = intel_ipc_dev_get(INTEL_SCU_IPC_DEV);
>> + if (IS_ERR_OR_NULL(scu_ipc_dev))
>> + return PTR_ERR(scu_ipc_dev);
>> +
>> watchdog_device.timer_tbl_ptr = sfi_get_mtmr(sfi_mtimer_num-1);
>>
>> if (watchdog_device.timer_tbl_ptr == NULL) {
>> --
>> 2.7.4
>>
--
Sathyanarayanan Kuppuswamy
Linux kernel developer
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: rtc: mediatek: add bindings for MediaTek SoC based RTC
From: Rob Herring @ 2017-10-03 21:58 UTC (permalink / raw)
To: sean.wang
Cc: a.zummo, alexandre.belloni, mark.rutland, linux-rtc, devicetree,
linux-mediatek, linux-kernel
In-Reply-To: <1357cb02adb88a8a2d7c2268b8573c0a9b6f8d0c.1506049341.git.sean.wang@mediatek.com>
On Fri, Sep 22, 2017 at 11:33:14AM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
>
> Add device-tree binding for MediaTek SoC based RTC
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
> .../devicetree/bindings/rtc/rtc-mediatek.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mediatek.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] rtc: armada38x: add support for trimming the RTC
From: Russell King - ARM Linux @ 2017-10-03 13:56 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Alessandro Zummo, Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, linux-arm-kernel, linux-rtc
In-Reply-To: <20171003132433.vkgjxuoeq26x3f47@piout.net>
On Tue, Oct 03, 2017 at 03:24:33PM +0200, Alexandre Belloni wrote:
> Hi Russell,
>
> On 29/09/2017 at 11:23:31 +0100, Russell King wrote:
> > +/*
> > + * The information given in the Armada 388 functional spec is complex.
> > + * They give two different formulas for calculating the offset value,
> > + * but when considering "Offset" as an 8-bit signed integer, they both
> > + * reduce down to (we shall rename "Offset" as "val" here):
> > + *
> > + * val = (f_ideal / f_measured - 1) / resolution where f_ideal = 32768
> > + *
> > + * Converting to time, f = 1/t:
> > + * val = (t_measured / t_ideal - 1) / resolution where t_ideal = 1/32768
> > + *
> > + * => t_measured / t_ideal = val * resolution + 1
> > + *
> > + * "offset" in the RTC interface is defined as:
> > + * t = t0 * (1 + offset * 1e-9)
> > + * where t is the desired period, t0 is the measured period with a zero
> > + * offset, which is t_measured above. With t0 = t_measured and t = t_ideal,
> > + * offset = (t_ideal / t_measured - 1) / 1e-9
> > + *
> > + * => t_ideal / t_measured = offset * 1e-9 + 1
> > + *
> > + * so:
> > + *
> > + * offset * 1e-9 + 1 = 1 / (val * resolution + 1)
> > + *
> > + * We want "resolution" to be an integer, so resolution = R * 1e-9, giving
> > + * offset = 1e18 / (val * R + 1e9) - 1e9
> > + * val = (1e18 / (offset + 1e9) - 1e9) / R
> > + * with a common transformation:
> > + * f(x) = 1e18 / (x + 1e9) - 1e9
> > + * offset = f(val * R)
> > + * val = f(offset) / R
> > + *
> > + * Armada 38x supports two modes, fine mode (954ppb) and coarse mode (3815ppb).
> > + */
> > +static long armada38x_ppb_convert(long ppb)
> > +{
> > + long div = ppb + 1000000000L;
> > +
> > + return div_s64(1000000000000000000LL + div / 2, div) - 1000000000L;
>
> The previous comment is perfect but it was not completely obvious to me
> why you are adding div / 2. Maybe you can add a small comment.
It's the standard way to turn a regular integer division into one which
rounds-to-closest - I consider it to be one of the basics which every
programmer should know, and therefore should be obvious.
Integer division in the form of:
r = (a + b / 2) / b
should be part of the programmers basic knowledge set.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH] rtc: clarify the RTC offset correction
From: Alexandre Belloni @ 2017-10-03 13:31 UTC (permalink / raw)
To: Russell King; +Cc: Alessandro Zummo, linux-rtc
In-Reply-To: <E1dxsS5-0006RD-Sy@rmk-PC.armlinux.org.uk>
Hi,
On 29/09/2017 at 11:23:25 +0100, Russell King wrote:
> The RTC offset correction documentation is not very clear about the
> exact relationship between "offset" and the effect it has on the RTC.
> Supplement the documentation with an equation giving the relationship.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/rtc/interface.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 8cec9a02c0b8..045e0a72d14b 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -1004,6 +1004,10 @@ int rtc_read_offset(struct rtc_device *rtc, long *offset)
> * to compensate for differences in the actual clock rate due to temperature,
> * the crystal, capacitor, etc.
> *
> + * The adjustment applied is as follows:
> + * t = t0 * (1 + offset * 1e-9)
> + * where t0 is the measured length of 1 RTC second with offset = 0
> + *
More documentation is available in Documentation/rtc.txt. Maybe it is
worth having the formula in both.
> * Kernel interface to adjust an rtc clock offset.
> * Return 0 on success, or a negative number on error.
> * If the rtc offset is not setable (or not implemented), return -EINVAL
> --
> 2.7.4
>
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH] rtc: armada38x: add support for trimming the RTC
From: Alexandre Belloni @ 2017-10-03 13:24 UTC (permalink / raw)
To: Russell King
Cc: Alessandro Zummo, Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, linux-arm-kernel, linux-rtc
In-Reply-To: <E1dxsSB-0006RM-08@rmk-PC.armlinux.org.uk>
Hi Russell,
On 29/09/2017 at 11:23:31 +0100, Russell King wrote:
> +/*
> + * The information given in the Armada 388 functional spec is complex.
> + * They give two different formulas for calculating the offset value,
> + * but when considering "Offset" as an 8-bit signed integer, they both
> + * reduce down to (we shall rename "Offset" as "val" here):
> + *
> + * val = (f_ideal / f_measured - 1) / resolution where f_ideal = 32768
> + *
> + * Converting to time, f = 1/t:
> + * val = (t_measured / t_ideal - 1) / resolution where t_ideal = 1/32768
> + *
> + * => t_measured / t_ideal = val * resolution + 1
> + *
> + * "offset" in the RTC interface is defined as:
> + * t = t0 * (1 + offset * 1e-9)
> + * where t is the desired period, t0 is the measured period with a zero
> + * offset, which is t_measured above. With t0 = t_measured and t = t_ideal,
> + * offset = (t_ideal / t_measured - 1) / 1e-9
> + *
> + * => t_ideal / t_measured = offset * 1e-9 + 1
> + *
> + * so:
> + *
> + * offset * 1e-9 + 1 = 1 / (val * resolution + 1)
> + *
> + * We want "resolution" to be an integer, so resolution = R * 1e-9, giving
> + * offset = 1e18 / (val * R + 1e9) - 1e9
> + * val = (1e18 / (offset + 1e9) - 1e9) / R
> + * with a common transformation:
> + * f(x) = 1e18 / (x + 1e9) - 1e9
> + * offset = f(val * R)
> + * val = f(offset) / R
> + *
> + * Armada 38x supports two modes, fine mode (954ppb) and coarse mode (3815ppb).
> + */
> +static long armada38x_ppb_convert(long ppb)
> +{
> + long div = ppb + 1000000000L;
> +
> + return div_s64(1000000000000000000LL + div / 2, div) - 1000000000L;
The previous comment is perfect but it was not completely obvious to me
why you are adding div / 2. Maybe you can add a small comment.
> +}
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v2 0/4] Make PL031 interrupt optional
From: Linus Walleij @ 2017-10-01 15:03 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Alessandro Zummo, linux-rtc, Alexandre Belloni,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170929102116.GX20805@n2100.armlinux.org.uk>
On Fri, Sep 29, 2017 at 12:21 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> There are some boards out there which have a PL031 RTC, but its
> interrupt is not wired up. To support these, we need the PL031
> to support the primecell without interrupts.
>
> When no interrupt is present, there's little point exposing the
> RTC's alarm capabilities, so we omit the alarm-related function
> calls - the RTC merely becomes a source of time-of-day.
>
> This patch series cleans up the pl031 driver a little, and adds
> support for this configuration.
This series:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [RFC v3 4/7] platform: x86: Add generic Intel IPC driver
From: Andy Shevchenko @ 2017-10-01 14:59 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <7d1a79925df8d2e35cfbd6974706d0d1ef0fc0de.1504588701.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Tue, Sep 5, 2017 at 8:37 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently intel_scu_ipc.c, intel_pmc_ipc.c and intel_punit_ipc.c
> redundantly implements the same IPC features and has lot of code
> duplication between them. This driver addresses this issue by grouping
> the common IPC functionalities under the same driver.
> +static const char *ipc_dev_err_string(struct intel_ipc_dev *ipc_dev,
> + int error)
> +{
> + switch (error) {
> + case IPC_DEV_ERR_NONE:
> + return "No error";
> + case IPC_DEV_ERR_CMD_NOT_SUPPORTED:
> + return "Command not-supported/Invalid";
> + case IPC_DEV_ERR_CMD_NOT_SERVICED:
> + return "Command not-serviced/Invalid param";
> + case IPC_DEV_ERR_UNABLE_TO_SERVICE:
> + return "Unable-to-service/Cmd-timeout";
> + case IPC_DEV_ERR_CMD_INVALID:
> + return "Command-invalid/Cmd-locked";
> + case IPC_DEV_ERR_CMD_FAILED:
> + return "Command-failed/Invalid-VR-id";
> + case IPC_DEV_ERR_EMSECURITY:
> + return "Invalid Battery/VR-Error";
> + case IPC_DEV_ERR_UNSIGNEDKERNEL:
> + return "Unsigned kernel";
> + default:
> + return "Unknown Command";
> + };
> +}
Since it's continuous list you can define an array of messages like
const char * const *errors = {
[..._ERR_...] = "",
...
};
Also you can use enum in the header and define _ERR_MAX there.
Thus, code would be transformed to
if (error < _ERR_MAX)
return errors[error];
return "Unknown Command";
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v3 3/7] platform/x86: intel_pmc_ipc: Use regmap calls for GCR updates
From: Andy Shevchenko @ 2017-10-01 14:48 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <5286489131571f149c6b75a8367ceb93cfe6e6be.1504588701.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Tue, Sep 5, 2017 at 8:37 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently, update_no_reboot_bit() function implemented in this driver
> uses mutex_lock to protect its register updates. But this function is
> called with in atomic context in iTCO_wdt_start() and iTCO_wdt_stop()
> functions in iTCO_wdt.c driver, which in turn causes "sleeping into
> atomic context" issue. This patch fixes this issue by refactoring the
> current GCR read/write/update functions with regmap APIs.
Since it sounds as candidate for stable, can we have split it to just
as less as possible intrusive fix + moving to regmap as a separate
change?
It should include Fixes: tag as well I suppose.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/intel_pmc_ipc.c | 115 ++++++++++++-----------------------
> 2 files changed, 40 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 80b8795..45f4e79 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1054,6 +1054,7 @@ config PVPANIC
> config INTEL_PMC_IPC
> tristate "Intel PMC IPC Driver"
> depends on ACPI
> + select REGMAP_MMIO
> ---help---
> This driver provides support for PMC control on some Intel platforms.
> The PMC is an ARC processor which defines IPC commands for communication
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index 021dcf6..40a25f8 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -31,9 +31,11 @@
> #include <linux/atomic.h>
> #include <linux/notifier.h>
> #include <linux/suspend.h>
> +#include <linux/spinlock.h>
> #include <linux/acpi.h>
> #include <linux/io-64-nonatomic-lo-hi.h>
> #include <linux/mfd/core.h>
> +#include <linux/regmap.h>
>
> #include <asm/intel_pmc_ipc.h>
>
> @@ -125,7 +127,7 @@ static struct intel_pmc_ipc_dev {
>
> /* gcr */
> void __iomem *gcr_mem_base;
> - bool has_gcr_regs;
> + struct regmap *gcr_regs;
>
> /* Telemetry */
> u8 telem_res_inval;
> @@ -150,6 +152,14 @@ static char *ipc_err_sources[] = {
> "Unsigned kernel",
> };
>
> +static struct regmap_config gcr_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + .max_register = PLAT_RESOURCE_GCR_SIZE,
> +};
> +
> /* Prevent concurrent calls to the PMC */
> static DEFINE_MUTEX(ipclock);
>
> @@ -183,21 +193,6 @@ static inline u32 ipc_data_readl(u32 offset)
> return readl(ipcdev.ipc_base + IPC_READ_BUFFER + offset);
> }
>
> -static inline u64 gcr_data_readq(u32 offset)
> -{
> - return readq(ipcdev.gcr_mem_base + offset);
> -}
> -
> -static inline int is_gcr_valid(u32 offset)
> -{
> - if (!ipcdev.has_gcr_regs)
> - return -EACCES;
> -
> - if (offset > PLAT_RESOURCE_GCR_SIZE)
> - return -EINVAL;
> -
> - return 0;
> -}
>
> /**
> * intel_pmc_gcr_read() - Read PMC GCR register
> @@ -210,21 +205,10 @@ static inline int is_gcr_valid(u32 offset)
> */
> int intel_pmc_gcr_read(u32 offset, u32 *data)
> {
> - int ret;
> -
> - mutex_lock(&ipclock);
> -
> - ret = is_gcr_valid(offset);
> - if (ret < 0) {
> - mutex_unlock(&ipclock);
> - return ret;
> - }
> -
> - *data = readl(ipcdev.gcr_mem_base + offset);
> -
> - mutex_unlock(&ipclock);
> + if (!ipcdev.gcr_regs)
> + return -EACCES;
>
> - return 0;
> + return regmap_read(ipcdev.gcr_regs, offset, data);
> }
> EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
>
> @@ -240,21 +224,10 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
> */
> int intel_pmc_gcr_write(u32 offset, u32 data)
> {
> - int ret;
> -
> - mutex_lock(&ipclock);
> -
> - ret = is_gcr_valid(offset);
> - if (ret < 0) {
> - mutex_unlock(&ipclock);
> - return ret;
> - }
> -
> - writel(data, ipcdev.gcr_mem_base + offset);
> -
> - mutex_unlock(&ipclock);
> + if (!ipcdev.gcr_regs)
> + return -EACCES;
>
> - return 0;
> + return regmap_write(ipcdev.gcr_regs, offset, data);
> }
> EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
>
> @@ -271,33 +244,10 @@ EXPORT_SYMBOL_GPL(intel_pmc_gcr_write);
> */
> int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val)
> {
> - u32 new_val;
> - int ret = 0;
> -
> - mutex_lock(&ipclock);
> -
> - ret = is_gcr_valid(offset);
> - if (ret < 0)
> - goto gcr_ipc_unlock;
> -
> - new_val = readl(ipcdev.gcr_mem_base + offset);
> -
> - new_val &= ~mask;
> - new_val |= val & mask;
> -
> - writel(new_val, ipcdev.gcr_mem_base + offset);
> -
> - new_val = readl(ipcdev.gcr_mem_base + offset);
> -
> - /* check whether the bit update is successful */
> - if ((new_val & mask) != (val & mask)) {
> - ret = -EIO;
> - goto gcr_ipc_unlock;
> - }
> + if (!ipcdev.gcr_regs)
> + return -EACCES;
>
> -gcr_ipc_unlock:
> - mutex_unlock(&ipclock);
> - return ret;
> + return regmap_update_bits(ipcdev.gcr_regs, offset, mask, val);
> }
> EXPORT_SYMBOL_GPL(intel_pmc_gcr_update);
>
> @@ -776,16 +726,24 @@ static int ipc_plat_get_res(struct platform_device *pdev)
> int intel_pmc_s0ix_counter_read(u64 *data)
> {
> u64 deep, shlw;
> + int ret;
>
> - if (!ipcdev.has_gcr_regs)
> + if (!ipcdev.gcr_regs)
> return -EACCES;
>
> - deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
> - shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
> + ret = regmap_bulk_read(ipcdev.gcr_regs, PMC_GCR_TELEM_DEEP_S0IX_REG,
> + &deep, 2);
> + if (ret)
> + return ret;
> +
> + ret = regmap_bulk_read(ipcdev.gcr_regs, PMC_GCR_TELEM_SHLW_S0IX_REG,
> + &shlw, 2);
> + if (ret)
> + return ret;
>
> *data = S0IX_RESIDENCY_IN_USECS(deep, shlw);
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(intel_pmc_s0ix_counter_read);
>
> @@ -817,6 +775,13 @@ static int ipc_plat_probe(struct platform_device *pdev)
> return ret;
> }
>
> + ipcdev.gcr_regs = devm_regmap_init_mmio_clk(ipcdev.dev, NULL,
> + ipcdev.gcr_mem_base, &gcr_regmap_config);
> + if (IS_ERR(ipcdev.gcr_regs)) {
> + dev_err(ipcdev.dev, "gcr_regs regmap init failed\n");
> + return PTR_ERR(ipcdev.gcr_regs);;
> + }
> +
> ret = ipc_create_pmc_devices(pdev);
> if (ret) {
> dev_err(&pdev->dev, "Failed to create pmc devices\n");
> @@ -836,8 +801,6 @@ static int ipc_plat_probe(struct platform_device *pdev)
> return ret;
> }
>
> - ipcdev.has_gcr_regs = true;
> -
> return 0;
> }
>
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v3 0/7] PMC/PUNIT IPC driver cleanup
From: Andy Shevchenko @ 2017-10-01 14:46 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <cover.1504588701.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Tue, Sep 5, 2017 at 8:37 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.=
com>
>
> Hi All,
>
> Currently intel_pmc_ipc.c, intel_punit_ipc.c, intel_scu_ipc.c drivers imp=
lements the same IPC features.
> This code duplication could be avoided if we implement the IPC driver as =
a generic library and let custom
> device drivers use API provided by generic driver. This patchset mainly a=
ddresses this issue.
>
> Along with above code duplication issue, This patchset also addresses fol=
lowing issues in intel_pmc_ipc and
> intel_punit_ipc drivers.
>
> 1. Intel_pmc_ipc.c driver does not use any resource managed (devm_*) call=
s.
> 2. In Intel_pmc_ipc.c driver, dependent devices like PUNIT, Telemetry and=
iTCO are created manually and uses lot of redundant buffer code.
> 3. Global variable is used to store the IPC device structure and it is us=
ed across all functions in intel_pmc_ipc.c and intel_punit_ipc.c.
>
> More info on Intel IPC device library:
> -------------------------------------
>
> A generic Intel IPC class driver has been implemented and all common IPC =
helper functions has been moved to this driver. It exposes APIs to create I=
PC device channel, send raw IPC command and simple IPC commands. It also cr=
eates device attribute to send IPC command from user space.
>
> API for creating a new IPC channel device is,
>
> struct intel_ipc_dev *devm_intel_ipc_dev_create(struct device *dev, const=
char *devname, struct intel_ipc_dev_cfg *cfg, struct intel_ipc_dev_ops *op=
s)
>
> The IPC channel drivers (PUNIT/PMC/SCU) when creating a new device can co=
nfigure their device params like register mapping, irq, irq-mode, channel t=
ype,etc using intel_ipc_dev_cfg and intel_ipc_dev_ops arguments. After a n=
ew IPC channel device is created, IPC users can use the generic APIs to mak=
e IPC calls.
>
> For example, after using this new model, IPC call to PMC device will look=
like,
>
> pmc_ipc_dev =3D intel_ipc_dev_get(INTEL_PMC_IPC_DEV);
> ipc_dev_raw_cmd(pmc_ipc_dev, cmd, PMC_PARAM_LEN, (u32 *)ipc_in, 1, NULL, =
0, 0, 0);
>
> I am still testing the driver in different products. But posted it to get=
some early comments. I also welcome any PMC/PUNIT driver users to check th=
ese patches in their product.
I have applied first patch from the series with some small changes.
Please rebase the rest on top of my review branch (review-andy).
Before sending new version, address comments that others and me did.
--=20
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v3 2/7] platform/x86: intel_pmc_ipc: Use MFD framework to create dependent devices
From: Andy Shevchenko @ 2017-10-01 14:44 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <3e7a05561840a7dd4fa945e4969edb834d9870b6.1504588701.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Tue, Sep 5, 2017 at 8:37 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Currently, we have lot of repetitive code in dependent device resource
> allocation and device creation handling code. This logic can be improved if
> we use MFD framework for dependent device creation. This patch adds this
> support.
>
> + punit_cell.id = -1;
> + return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
> + &punit_cell, 1, NULL, 0, NULL);
IIRC you don't need to file cell ID in case of DEVID_AUTO.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC v3 1/7] platform/x86: intel_pmc_ipc: Use devm_* calls in driver probe function
From: Andy Shevchenko @ 2017-10-01 14:34 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Alessandro Zummo, x86@kernel.org, Wim Van Sebroeck, Ingo Molnar,
Alexandre Belloni, Zha Qipeng, H. Peter Anvin,
dvhart@infradead.org, Thomas Gleixner, Lee Jones, Andy Shevchenko,
Souvik Kumar Chakravarty, linux-rtc, linux-watchdog,
linux-kernel@vger.kernel.org, Platform Driver,
Sathyanarayanan Kuppuswamy Natarajan
In-Reply-To: <eada94e0dfe0302b22c16c22ba2ea806e096144a.1504588701.git.sathyanarayanan.kuppuswamy@linux.intel.com>
On Tue, Sep 5, 2017 at 8:37 AM,
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> This patch cleans up unnecessary free/alloc calls in ipc_plat_probe(),
> ipc_pci_probe() and ipc_plat_get_res() functions by using devm_*
> calls.
>
> This patch also adds proper error handling for failure cases in
> ipc_pci_probe() function.
>
Pushed (this patch only) to my review queue with slight style changes, thanks.
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> drivers/platform/x86/intel_pmc_ipc.c | 104 ++++++++++++-----------------------
> 1 file changed, 34 insertions(+), 70 deletions(-)
>
> Changes since v2:
> * Used pcim_* device managed functions.
>
> Changes since v1:
> * Merged devm_* related changes into a single function.
> * Instead of removing free_irq, use devm_free_irq function.
>
> diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
> index bb792a5..5eef649 100644
> --- a/drivers/platform/x86/intel_pmc_ipc.c
> +++ b/drivers/platform/x86/intel_pmc_ipc.c
> @@ -480,52 +480,39 @@ static irqreturn_t ioc(int irq, void *dev_id)
>
> static int ipc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> - resource_size_t pci_resource;
> int ret;
> - int len;
> + struct intel_pmc_ipc_dev *pmc = &ipcdev;
>
> - ipcdev.dev = &pci_dev_get(pdev)->dev;
> - ipcdev.irq_mode = IPC_TRIGGER_MODE_IRQ;
> + /* Only one PMC is supported */
> + if (pmc->dev)
> + return -EBUSY;
>
> - ret = pci_enable_device(pdev);
> + pmc->irq_mode = IPC_TRIGGER_MODE_IRQ;
> +
> + ret = pcim_enable_device(pdev);
> if (ret)
> return ret;
>
> - ret = pci_request_regions(pdev, "intel_pmc_ipc");
> + ret = pcim_iomap_regions(pdev, 1 << 0, pci_name(pdev));
> if (ret)
> return ret;
>
> - pci_resource = pci_resource_start(pdev, 0);
> - len = pci_resource_len(pdev, 0);
> - if (!pci_resource || !len) {
> - dev_err(&pdev->dev, "Failed to get resource\n");
> - return -ENOMEM;
> - }
> + init_completion(&pmc->cmd_complete);
>
> - init_completion(&ipcdev.cmd_complete);
> + pmc->ipc_base = pcim_iomap_table(pdev)[0];
>
> - if (request_irq(pdev->irq, ioc, 0, "intel_pmc_ipc", &ipcdev)) {
> + ret = devm_request_irq(&pdev->dev, pdev->irq, ioc, 0, "intel_pmc_ipc",
> + pmc);
> + if (ret) {
> dev_err(&pdev->dev, "Failed to request irq\n");
> - return -EBUSY;
> + return ret;
> }
>
> - ipcdev.ipc_base = ioremap_nocache(pci_resource, len);
> - if (!ipcdev.ipc_base) {
> - dev_err(&pdev->dev, "Failed to ioremap ipc base\n");
> - free_irq(pdev->irq, &ipcdev);
> - ret = -ENOMEM;
> - }
> + pmc->dev = &pdev->dev;
>
> - return ret;
> -}
> + pci_set_drvdata(pdev, pmc);
>
> -static void ipc_pci_remove(struct pci_dev *pdev)
> -{
> - free_irq(pdev->irq, &ipcdev);
> - pci_release_regions(pdev);
> - pci_dev_put(pdev);
> - iounmap(ipcdev.ipc_base);
> - ipcdev.dev = NULL;
> + return 0;
> }
>
> static const struct pci_device_id ipc_pci_ids[] = {
> @@ -540,7 +527,6 @@ static struct pci_driver ipc_pci_driver = {
> .name = "intel_pmc_ipc",
> .id_table = ipc_pci_ids,
> .probe = ipc_pci_probe,
> - .remove = ipc_pci_remove,
> };
>
> static ssize_t intel_pmc_ipc_simple_cmd_store(struct device *dev,
> @@ -849,18 +835,16 @@ static int ipc_plat_get_res(struct platform_device *pdev)
> dev_err(&pdev->dev, "Failed to get ipc resource\n");
> return -ENXIO;
> }
> - size = PLAT_RESOURCE_IPC_SIZE + PLAT_RESOURCE_GCR_SIZE;
> + res->end = (res->start + PLAT_RESOURCE_IPC_SIZE +
> + PLAT_RESOURCE_GCR_SIZE - 1);
>
> - if (!request_mem_region(res->start, size, pdev->name)) {
> - dev_err(&pdev->dev, "Failed to request ipc resource\n");
> - return -EBUSY;
> - }
> - addr = ioremap_nocache(res->start, size);
> - if (!addr) {
> - dev_err(&pdev->dev, "I/O memory remapping failed\n");
> - release_mem_region(res->start, size);
> - return -ENOMEM;
> + addr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(addr)) {
> + dev_err(&pdev->dev,
> + "PMC I/O memory remapping failed\n");
> + return PTR_ERR(addr);
> }
> +
> ipcdev.ipc_base = addr;
>
> ipcdev.gcr_mem_base = addr + PLAT_RESOURCE_GCR_OFFSET;
> @@ -917,7 +901,6 @@ MODULE_DEVICE_TABLE(acpi, ipc_acpi_ids);
>
> static int ipc_plat_probe(struct platform_device *pdev)
> {
> - struct resource *res;
> int ret;
>
> ipcdev.dev = &pdev->dev;
> @@ -939,61 +922,42 @@ static int ipc_plat_probe(struct platform_device *pdev)
> ret = ipc_create_pmc_devices();
> if (ret) {
> dev_err(&pdev->dev, "Failed to create pmc devices\n");
> - goto err_device;
> + return ret;
> }
>
> - if (request_irq(ipcdev.irq, ioc, IRQF_NO_SUSPEND,
> - "intel_pmc_ipc", &ipcdev)) {
> + if (devm_request_irq(&pdev->dev, ipcdev.irq, ioc, IRQF_NO_SUSPEND,
> + "intel_pmc_ipc", &ipcdev)) {
> dev_err(&pdev->dev, "Failed to request irq\n");
> ret = -EBUSY;
> - goto err_irq;
> + goto unregister_devices;
> }
>
> ret = sysfs_create_group(&pdev->dev.kobj, &intel_ipc_group);
> if (ret) {
> dev_err(&pdev->dev, "Failed to create sysfs group %d\n",
> ret);
> - goto err_sys;
> + goto unregister_devices;
> }
>
> ipcdev.has_gcr_regs = true;
>
> return 0;
> -err_sys:
> - free_irq(ipcdev.irq, &ipcdev);
> -err_irq:
> +
> +unregister_devices:
> platform_device_unregister(ipcdev.tco_dev);
> platform_device_unregister(ipcdev.punit_dev);
> platform_device_unregister(ipcdev.telemetry_dev);
> -err_device:
> - iounmap(ipcdev.ipc_base);
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_IPC_INDEX);
> - if (res) {
> - release_mem_region(res->start,
> - PLAT_RESOURCE_IPC_SIZE +
> - PLAT_RESOURCE_GCR_SIZE);
> - }
> +
> return ret;
> }
>
> static int ipc_plat_remove(struct platform_device *pdev)
> {
> - struct resource *res;
> -
> sysfs_remove_group(&pdev->dev.kobj, &intel_ipc_group);
> - free_irq(ipcdev.irq, &ipcdev);
> + devm_free_irq(&pdev->dev, ipcdev.irq, &ipcdev);
> platform_device_unregister(ipcdev.tco_dev);
> platform_device_unregister(ipcdev.punit_dev);
> platform_device_unregister(ipcdev.telemetry_dev);
> - iounmap(ipcdev.ipc_base);
> - res = platform_get_resource(pdev, IORESOURCE_MEM,
> - PLAT_RESOURCE_IPC_INDEX);
> - if (res) {
> - release_mem_region(res->start,
> - PLAT_RESOURCE_IPC_SIZE +
> - PLAT_RESOURCE_GCR_SIZE);
> - }
> ipcdev.dev = NULL;
> return 0;
> }
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox