* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 13:32 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Alex Courbot, Grant Likely, Anton Vorontsov,
Stephen Warren, Mark Zhang, Rob Herring, Mark Brown,
David Woodhouse, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot, Laurent Pinchart
In-Reply-To: <20121121130039.GA12191-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]
On 2012-11-21 15:00, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>> On 2012-11-21 13:40, Thierry Reding wrote:
>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>
>> (sorry for bouncing back and forth with my private and my @ti addresses.
>> I can't find an option in thunderbird to only use one sender address,
>> and I always forget to change it when responding...)
>>
>>>> My suggestion would be to go forward with an in-driver solution, and
>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>> in the drivers.
>>>
>>> Assuming we go with your approach, what's the plan? We're actually
>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>> that can drive the panel, but we're still missing a way to hook up the
>>> backlight and panel enabling code. So we effectively can't support any
>>> of the LVDS devices out there without this series.
>>
>> Could you describe the hardware setup you have related to the LCD and
>> backlight? Is it a public board with public schematics?
>
> I don't think any of the schematics are public. The Tamonten Evaluation
> Carrier is available publicly from our website and the schematics are
> available on demand as well. If required I can probably arrange to send
> you a copy.
No need, I think your answer below is enough.
>> I've understood that you don't have anything special in your board, just
>> an LCD and a backlight, and the power sequences are related to powering
>> up the LCD and the backlight, without anything board specific. If so,
>> there's no need for board specific code, but just improving the panel
>> and backlight drivers to support the models you use.
>
> Correct. Basically we have two GPIOs that each enable the panel or the
> backlight respectively and one PWM to control the brightness. Are the
The panel GPIO goes to the panel hardware device, and enables the panel?
And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
making sure there are no other components involved.
> panel drivers that you refer to those in drivers/video? I'm not sure if
> adding more ad-hoc drivers there just to move them to a generic
> framework in the next cycle is a good idea. I'd rather spend some time
> on helping to get the framework right and have drivers for that instead.
We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
sure if other platforms have their own versions of panel drivers, but
probably adding a simple panel driver system for a platform would not be
too difficult. It could even be quite hardcoded, i.e. embedded directly
into the display subsystem driver, just to get something working until
the common panel framework is available.
Yes, I agree it's not good idea to add more platform specific panel
drivers. But it's unclear when CPF will be merged, so if you need to get
the panel working now, I don't see a simple ad-hoc driver as too
horrible. But, of course, I'm not the one making the decision whether to
merge or not =).
> From what I understand by looking at the OMAP display drivers, they also
> provide the timings for the displays. Steffen's videomode helpers can be
> used to represent these easily in DT, but I suppose if all of those per-
Right. Note that I didn't present omap panel drivers as perfect
examples, just examples =).
> panel specifics are represented in the drivers then that won't be needed
> anymore either.
Yes, for most panels with just one native mode and nothing else, the
panel driver can contain the timings.
However, this subject has been discussed earlier a few times. If the
panel in question doesn't need any special power-on/off sequences, just,
perhaps, one gpio or such, we could still use DT video modes. This would
simplify the cases where you have lots of different very simple panels.
Obviously the same questions apply to DT video modes than to the power
sequences, and while I do think it's better to handle the timings inside
the driver, I'm not too much against video timings in DT. The reason
being that the video modes are quite clear, simple and stable data,
versus the much more complex and open-ended power sequences.
>> I don't see any problem with adding small Tegra specific panel drivers
>> for the time being, with the intention of converting to common panel
>> framework when that's available.
>
> I can take a look at how such a driver could be implemented, but again,
Don't look at the mainline omap panel drivers, at least not too closely
=). They contain hackery that will be cleaned up with CPF.
I think there are two methods to implements simple panel drivers:
The hardcoded one, where the display subsystem driver manages a few
different panel models. This is obviously not very expandable or
"correct", but should probably work just fine for a few models, until
CPF is usable.
Something like CPF will have: have the panel device/driver as a platform
device/driver, which will register itself to the display subsystem. And
with "itself" I mean some kind of struct panel_entity, with a few ops
implemented by the panel driver.
Well, this goes a bit out of subject. If you want to discuss panel
drivers more, please start a new thread. Laurent's upcoming CPF v2
should give you good ideas what the model will be.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-11-21 13:00 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Tomi Valkeinen, Alex Courbot, Grant Likely, Anton Vorontsov,
Stephen Warren, Mark Zhang, Rob Herring, Mark Brown,
David Woodhouse, Arnd Bergmann, linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-pm@vger.kernel.org,
Alexandre Courbot, Laurent Pinchart
In-Reply-To: <50ACC341.3090204@ti.com>
[-- Attachment #1: Type: text/plain, Size: 7937 bytes --]
On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
> On 2012-11-21 13:40, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>
> (sorry for bouncing back and forth with my private and my @ti addresses.
> I can't find an option in thunderbird to only use one sender address,
> and I always forget to change it when responding...)
>
> >> My suggestion would be to go forward with an in-driver solution, and
> >> look at the DT based solution later if we are seeing an increasing bloat
> >> in the drivers.
> >
> > Assuming we go with your approach, what's the plan? We're actually
> > facing this problem right now for Tegra. Basically we have a DRM driver
> > that can drive the panel, but we're still missing a way to hook up the
> > backlight and panel enabling code. So we effectively can't support any
> > of the LVDS devices out there without this series.
>
> Could you describe the hardware setup you have related to the LCD and
> backlight? Is it a public board with public schematics?
I don't think any of the schematics are public. The Tamonten Evaluation
Carrier is available publicly from our website and the schematics are
available on demand as well. If required I can probably arrange to send
you a copy.
> I've understood that you don't have anything special in your board, just
> an LCD and a backlight, and the power sequences are related to powering
> up the LCD and the backlight, without anything board specific. If so,
> there's no need for board specific code, but just improving the panel
> and backlight drivers to support the models you use.
Correct. Basically we have two GPIOs that each enable the panel or the
backlight respectively and one PWM to control the brightness. Are the
panel drivers that you refer to those in drivers/video? I'm not sure if
adding more ad-hoc drivers there just to move them to a generic
framework in the next cycle is a good idea. I'd rather spend some time
on helping to get the framework right and have drivers for that instead.
From what I understand by looking at the OMAP display drivers, they also
provide the timings for the displays. Steffen's videomode helpers can be
used to represent these easily in DT, but I suppose if all of those per-
panel specifics are represented in the drivers then that won't be needed
anymore either.
> > As I understand it, what you propose is similar to what ASoC does. For a
> > specific board, you'd have to write a driver, presumably for the new
> > panel/display framework, that provides code to power the panel on and
> > off. That means we'll have to have a driver for each panel out there
> > basically, or we'd need to write generic drivers that can be configured
> > to some degree (via platform data or DT). This is similar to how ASoC
> > works, where we have a driver that provides support for a specific codec
> > connected to the Tegra SoC. For the display framework things could be
> > done in a similar way I suppose, so that Tegra could have one display
> > driver to handle all aspects of powering on and off the various panels
> > for the various boards out there.
>
> I think we should only need the board drivers for very special cases. If
> there's just a panel and a backlight, without any special dynamic muxing
> or other trickery needed, I don't see a need for a board driver. I
> presume this is the case for most of the boards.
For Tegra ASoC, the way to provide for this is to allow a specific board
to introduce a separate compatible value to enable per-board quirks or
special handling if it cannot be supported by the generic driver and
configuration mechanisms.
> > Obviously, a lot of the code will be similar for other SoCs, but maybe
> > that's just the way things are if we choose that approach. There's also
> > the potential for factoring out large chunks of common code later on
> > once we start to see common patterns.
> >
> > One thing that's not very clear is how the backlight subsystem should be
> > wired up with the display framework. I have a patch on top of the Tegra
> > DRM driver which adds some ad-hoc display support using this power
> > sequences series and the pwm-backlight.
>
> I think that's a separate issue: how to associate the lcd device and
> backlight device together. I don't have a clear answer to this.
>
> There are many ways the backlight may be handled. In some cases the
> panel and the backlight are truly independent, and you can use the other
> without using the other (not very practical, though =).
At least for DT I think we can easily wire that up. I've actually posted
a patch recently that does so. I think in most cases it makes sense to
control them together, such as on DPMS changes, where you really want to
turn both the backlight and the LCD off, independent of how they are
tied together.
> But then with some LCDs the backlight may be controlled by sending
> commands to the panel, and in this case the two may be quite linked.
> Changing the backlight requires the panel driver to be up and running,
> and sometimes the sending the backlight commands may need to be (say,
> DSI display, with backlight commands going over the DSI bus).
>
> So my feeling is that the panel driver should know about the related
> backlight device. In the first case the panel driver would just call
> enable/disable in the backlight device when the panel is turned on.
Exactly.
> In the second case of the DSI panel... I'm not sure. I've implemented it
> so that the panel driver creates the backlight device, and implements
> the backlight callbacks. It then sends the DSI commands from those
> callbacks.
That certainly sounds like the right approach to me.
> > From reading the proposal for the panel/display framework, it sounds
> > like a lot more is planned than just enabling or disabling panels, but
> > it also seems like a lot of code needs to be written to support things
> > like DSI, DBI or other control busses.
> >
> > At least for Tegra, and I think the same holds for a wide variety of
> > other SoCs, dumb panels would be enough for a start. In the interest of
> > getting a working solution for those setups, maybe we can start small
> > and add just enough framework to register dumb panel drivers to along
> > with code to wire up a backlight to light up the display. Then we could
> > possibly still make it to have a proper solution to support the various
> > LVDS panels for Tegra with 3.9.
>
> Yes, we (Laurent and me) both agree that we should start simple.
>
> However, the common panel framework is not strictly needed for this. I'm
> not sure of the current architecture for Tegra, but for OMAP we already
> have panel drivers (omap specific ones, though). The panel drivers may
> support multiple models, (for example,
> drivers/video/omap2/displays/panel-generic-dpi.c).
>
> I don't see any problem with adding small Tegra specific panel drivers
> for the time being, with the intention of converting to common panel
> framework when that's available.
I can take a look at how such a driver could be implemented, but again,
I'm a bit reluctant to add something ad-hoc now when maybe we can have
it supported in a proper framework not too far away in the future.
> Of course, the DT side is an issue. If you now create DT bindings for a
> temporary model, and need to change it again later, you'll have some
> headaches trying managing that without breaking the old bindings... This
> is why I haven't pushed DT bindings for OMAP, as I know I have to change
> them in the near future.
We're already keeping back on this and none of the patches that define
the bindings have been merged yet. Although bindings have been known to
change every once in a while even for code that is already in mainline.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* RE: [PATCH v3 RESEND 0/4] ARM: EXYNOS: Power domain DT support extension
From: Kukjin Kim @ 2012-11-21 12:15 UTC (permalink / raw)
To: 'Tomasz Figa', linux-samsung-soc
Cc: linux-pm, linux-arm-kernel, kyungmin.park, thomas.abraham, rjw,
m.szyprowski, tomasz.figa
In-Reply-To: <3937720.DSR9HIu3D4@amdc1227>
Tomasz Figa wrote:
>
> Hi Kgene,
>
> On Tuesday 13 of November 2012 14:08:23 Tomasz Figa wrote:
> > This patch series fixes two issues with existing DT support for Exynos
> > power domains and extends it with the ability of binding devices to
> > domains, basically making it possible to use power domains with DT.
> >
> > Based on for-next branch of Kgene's tree.
> >
> > Changes since v2:
> > - Rebased to for-next of Kgene's tree.
> > Changes since v1:
> > - Added "samsung," prefix to "power-domain" property.
> > - Added power domain nodes to Exynos4 device tree sources.
> >
> > Tomasz Figa (4):
> > ARM: EXYNOS: pm_domain: Detect domain state on registration from DT
> > ARM: EXYNOS: pm_domain: Fix power domain name initialization
> > ARM: EXYNOS: pm_domain: Bind devices to power domains using DT
> > ARM: dts: exynos4: Set up power domains
> >
> > .../bindings/arm/exynos/power_domain.txt | 15 +++-
> > arch/arm/boot/dts/exynos4.dtsi | 30 +++++++
> > arch/arm/boot/dts/exynos4210.dtsi | 5 ++
> > arch/arm/mach-exynos/pm_domains.c | 93
> > +++++++++++++++++++++- 4 files changed, 135 insertions(+), 8
> > deletions(-)
>
> Since this version is just a rebase to your for-next and there are no
> remaining comments, could you still take it for 3.8?
>
Well, I'm not sure 'S5P_INT_LOCAL_PWR_EN' in [1/4] patch is available on all
of exynos and it can support multiple platform. But at this moment, looks OK
to me. If any modification is required, we can do it later.
Will apply, thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 12:04 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Alex Courbot, Grant Likely, Anton Vorontsov,
Stephen Warren, Mark Zhang, Rob Herring, Mark Brown,
David Woodhouse, Arnd Bergmann, linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-pm@vger.kernel.org,
Alexandre Courbot, Laurent Pinchart
In-Reply-To: <20121121114018.GA31576@avionic-0098.adnet.avionic-design.de>
[-- Attachment #1: Type: text/plain, Size: 5646 bytes --]
On 2012-11-21 13:40, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
(sorry for bouncing back and forth with my private and my @ti addresses.
I can't find an option in thunderbird to only use one sender address,
and I always forget to change it when responding...)
>> My suggestion would be to go forward with an in-driver solution, and
>> look at the DT based solution later if we are seeing an increasing bloat
>> in the drivers.
>
> Assuming we go with your approach, what's the plan? We're actually
> facing this problem right now for Tegra. Basically we have a DRM driver
> that can drive the panel, but we're still missing a way to hook up the
> backlight and panel enabling code. So we effectively can't support any
> of the LVDS devices out there without this series.
Could you describe the hardware setup you have related to the LCD and
backlight? Is it a public board with public schematics?
I've understood that you don't have anything special in your board, just
an LCD and a backlight, and the power sequences are related to powering
up the LCD and the backlight, without anything board specific. If so,
there's no need for board specific code, but just improving the panel
and backlight drivers to support the models you use.
> As I understand it, what you propose is similar to what ASoC does. For a
> specific board, you'd have to write a driver, presumably for the new
> panel/display framework, that provides code to power the panel on and
> off. That means we'll have to have a driver for each panel out there
> basically, or we'd need to write generic drivers that can be configured
> to some degree (via platform data or DT). This is similar to how ASoC
> works, where we have a driver that provides support for a specific codec
> connected to the Tegra SoC. For the display framework things could be
> done in a similar way I suppose, so that Tegra could have one display
> driver to handle all aspects of powering on and off the various panels
> for the various boards out there.
I think we should only need the board drivers for very special cases. If
there's just a panel and a backlight, without any special dynamic muxing
or other trickery needed, I don't see a need for a board driver. I
presume this is the case for most of the boards.
> Obviously, a lot of the code will be similar for other SoCs, but maybe
> that's just the way things are if we choose that approach. There's also
> the potential for factoring out large chunks of common code later on
> once we start to see common patterns.
>
> One thing that's not very clear is how the backlight subsystem should be
> wired up with the display framework. I have a patch on top of the Tegra
> DRM driver which adds some ad-hoc display support using this power
> sequences series and the pwm-backlight.
I think that's a separate issue: how to associate the lcd device and
backlight device together. I don't have a clear answer to this.
There are many ways the backlight may be handled. In some cases the
panel and the backlight are truly independent, and you can use the other
without using the other (not very practical, though =).
But then with some LCDs the backlight may be controlled by sending
commands to the panel, and in this case the two may be quite linked.
Changing the backlight requires the panel driver to be up and running,
and sometimes the sending the backlight commands may need to be (say,
DSI display, with backlight commands going over the DSI bus).
So my feeling is that the panel driver should know about the related
backlight device. In the first case the panel driver would just call
enable/disable in the backlight device when the panel is turned on.
In the second case of the DSI panel... I'm not sure. I've implemented it
so that the panel driver creates the backlight device, and implements
the backlight callbacks. It then sends the DSI commands from those
callbacks.
> From reading the proposal for the panel/display framework, it sounds
> like a lot more is planned than just enabling or disabling panels, but
> it also seems like a lot of code needs to be written to support things
> like DSI, DBI or other control busses.
>
> At least for Tegra, and I think the same holds for a wide variety of
> other SoCs, dumb panels would be enough for a start. In the interest of
> getting a working solution for those setups, maybe we can start small
> and add just enough framework to register dumb panel drivers to along
> with code to wire up a backlight to light up the display. Then we could
> possibly still make it to have a proper solution to support the various
> LVDS panels for Tegra with 3.9.
Yes, we (Laurent and me) both agree that we should start simple.
However, the common panel framework is not strictly needed for this. I'm
not sure of the current architecture for Tegra, but for OMAP we already
have panel drivers (omap specific ones, though). The panel drivers may
support multiple models, (for example,
drivers/video/omap2/displays/panel-generic-dpi.c).
I don't see any problem with adding small Tegra specific panel drivers
for the time being, with the intention of converting to common panel
framework when that's available.
Of course, the DT side is an issue. If you now create DT bindings for a
temporary model, and need to change it again later, you'll have some
headaches trying managing that without breaking the old bindings... This
is why I haven't pushed DT bindings for OMAP, as I know I have to change
them in the near future.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-11-21 11:40 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Alex Courbot, Grant Likely, Anton Vorontsov, Stephen Warren,
Mark Zhang, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot, Laurent Pinchart
In-Reply-To: <50ACB59B.4090404-X3B1VOXEql0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 8080 bytes --]
On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
> On 2012-11-21 06:23, Alex Courbot wrote:
> > Hi Grant,
> >
> > On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> >>> With the advent of the device tree and of ARM kernels that are not
> >>> board-tied, we cannot rely on these board-specific hooks anymore but
> >>
> >> This isn't strictly true. It is still perfectly fine to have board
> >> specific code when necessary. However, there is strong encouragement to
> >> enable that code in device drivers as much as possible and new board
> >> files need to have very strong justification.
> >
> > But doesn't introducing board-specific code into the kernel just defeats the
> > purpose of the DT? If we extend this logic, we are heading straight back to
> > board-definition files. To a lesser extent than before I agree, but the problem
> > is fundamentally the same.
>
> I don't think so. I'll reiterate my opinion on this subject, as a
> summary and for those who haven't read the discussions of the earlier
> versions of the series. And perhaps I'll even manage to say something
> new =).
>
>
> First about the board specific code. I think we may need some board
> specific code, even if this series was merged. Let's call them board
> drivers. These board drivers would only exist for boards with some weird
> setups that cannot be expressed or managed with DT and normal drivers.
>
> I think these cases would be quite rare, as I can't even come up with a
> very good example. I guess most likely these cases would involve some
> small trivial chips for muxing or such, for which it doesn't really make
> sense to have a real driver.
>
> Say, perhaps a board with two LCDs connected to one video output, and
> only one LCD can be enabled at a time, and you need to set some mux chip
> to route the signals to the correct LCD. In this case I'd see we should
> have hotplug support in the display framework, and the board driver
> would act on user input (sysfs file, perhaps), plugging in/out the LCD
> device depending on the user input.
>
>
> As for expressing device internal details in the DT data, as done in
> this series, I don't like it very much. I think the DT data or the board
> file should just describe which device is the one in question, and how
> it's connected to other components on the board. The driver for the
> device should handle everything else.
>
> As Alex pointed out, there may be lots of devices that work the same way
> when enabled, but require slightly different power-on/off sequences. We
> could have these sequences in the driver itself, either as plain code,
> or in a table of some sort, possibly using the power sequence framework
> presented in this series. The correct code or sequence would be ran
> depending on the particular model of the device.
>
> I think this approach is correct in the sense that this is what drivers
> are supposed to do: handle all the device internal matters. But this
> raises the problem of bloating the kernel with possibly lots of
> different power sequences, of which only a few would be used by a board,
> and all the rest would just be waste of memory.
>
> Regarding this problem I have the following questions, to which I don't
> have clear answers:
>
> - How much of this device specific data is too much? If a driver
> supports 10 different models, and the sequences for each model take 100
> bytes, this 1000 bytes doesn't sound too much. But where's the limit?
> And even if one driver only has 1kB of this data, what if we have lots
> of similar drivers?
>
> - How many bytes does a power sequence presented in this series take, if
> the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and
> regulator?
>
> - How likely is it that we'll get lots of different models? A hundred
> different models for a backlight PWM with different power-on/off
> sequences already sounds a really big number. If we're only going to
> have each driver supporting, say, no more than 10 models, perhaps the
> problem is not even an issue in practice.
>
> - Are there ways to limit this bloat in the driver? One obvious way
> would be to discard the unused sequences after driver probe, but that
> only works for platform devices. Although, I guess these sequences would
> mostly be used by platform drivers? If so, then the problem could be
> solved by discarding the data after probe. Another would be to load the
> sequences from a file as firmware, but that feels quite an awful
> solution. Anybody have other ideas?
>
> One clear positive side with in-driver approach is that it's totally
> inside the kernel, and can be easily reworked in the future, or even
> changed to a DT-based approach as presented in this series if that seems
> like a better solution. The DT based approach, on the other hand, will
> be more or less written to stone after it's merged.
>
>
> So, I like the framework of expressing the sequences presented in this
> series (except there's a problem with error cases, as I pointed out in
> another post), as it can be used inside the drivers. But I'm not so
> enthusiastic about presenting the sequences in DT.
>
> My suggestion would be to go forward with an in-driver solution, and
> look at the DT based solution later if we are seeing an increasing bloat
> in the drivers.
Assuming we go with your approach, what's the plan? We're actually
facing this problem right now for Tegra. Basically we have a DRM driver
that can drive the panel, but we're still missing a way to hook up the
backlight and panel enabling code. So we effectively can't support any
of the LVDS devices out there without this series.
As I understand it, what you propose is similar to what ASoC does. For a
specific board, you'd have to write a driver, presumably for the new
panel/display framework, that provides code to power the panel on and
off. That means we'll have to have a driver for each panel out there
basically, or we'd need to write generic drivers that can be configured
to some degree (via platform data or DT). This is similar to how ASoC
works, where we have a driver that provides support for a specific codec
connected to the Tegra SoC. For the display framework things could be
done in a similar way I suppose, so that Tegra could have one display
driver to handle all aspects of powering on and off the various panels
for the various boards out there.
Obviously, a lot of the code will be similar for other SoCs, but maybe
that's just the way things are if we choose that approach. There's also
the potential for factoring out large chunks of common code later on
once we start to see common patterns.
One thing that's not very clear is how the backlight subsystem should be
wired up with the display framework. I have a patch on top of the Tegra
DRM driver which adds some ad-hoc display support using this power
sequences series and the pwm-backlight.
From reading the proposal for the panel/display framework, it sounds
like a lot more is planned than just enabling or disabling panels, but
it also seems like a lot of code needs to be written to support things
like DSI, DBI or other control busses.
At least for Tegra, and I think the same holds for a wide variety of
other SoCs, dumb panels would be enough for a start. In the interest of
getting a working solution for those setups, maybe we can start small
and add just enough framework to register dumb panel drivers to along
with code to wire up a backlight to light up the display. Then we could
possibly still make it to have a proper solution to support the various
LVDS panels for Tegra with 3.9.
I'm adding Laurent on Cc since he's probably busy working on a new
proposal for the panel/display framework. Maybe he can share his thought
on this.
All of the above said, I'm willing to help out with the coding if that's
what is required to reach a solution that everybody can be happy with.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 11:06 UTC (permalink / raw)
To: Alex Courbot
Cc: Grant Likely, Anton Vorontsov, Stephen Warren, Thierry Reding,
Mark Zhang, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot
In-Reply-To: <13540495.epaCf4JVn9@percival>
On 2012-11-21 06:23, Alex Courbot wrote:
> Hi Grant,
>
> On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
>>> With the advent of the device tree and of ARM kernels that are not
>>> board-tied, we cannot rely on these board-specific hooks anymore but
>>
>> This isn't strictly true. It is still perfectly fine to have board
>> specific code when necessary. However, there is strong encouragement to
>> enable that code in device drivers as much as possible and new board
>> files need to have very strong justification.
>
> But doesn't introducing board-specific code into the kernel just defeats the
> purpose of the DT? If we extend this logic, we are heading straight back to
> board-definition files. To a lesser extent than before I agree, but the problem
> is fundamentally the same.
I don't think so. I'll reiterate my opinion on this subject, as a
summary and for those who haven't read the discussions of the earlier
versions of the series. And perhaps I'll even manage to say something
new =).
First about the board specific code. I think we may need some board
specific code, even if this series was merged. Let's call them board
drivers. These board drivers would only exist for boards with some weird
setups that cannot be expressed or managed with DT and normal drivers.
I think these cases would be quite rare, as I can't even come up with a
very good example. I guess most likely these cases would involve some
small trivial chips for muxing or such, for which it doesn't really make
sense to have a real driver.
Say, perhaps a board with two LCDs connected to one video output, and
only one LCD can be enabled at a time, and you need to set some mux chip
to route the signals to the correct LCD. In this case I'd see we should
have hotplug support in the display framework, and the board driver
would act on user input (sysfs file, perhaps), plugging in/out the LCD
device depending on the user input.
As for expressing device internal details in the DT data, as done in
this series, I don't like it very much. I think the DT data or the board
file should just describe which device is the one in question, and how
it's connected to other components on the board. The driver for the
device should handle everything else.
As Alex pointed out, there may be lots of devices that work the same way
when enabled, but require slightly different power-on/off sequences. We
could have these sequences in the driver itself, either as plain code,
or in a table of some sort, possibly using the power sequence framework
presented in this series. The correct code or sequence would be ran
depending on the particular model of the device.
I think this approach is correct in the sense that this is what drivers
are supposed to do: handle all the device internal matters. But this
raises the problem of bloating the kernel with possibly lots of
different power sequences, of which only a few would be used by a board,
and all the rest would just be waste of memory.
Regarding this problem I have the following questions, to which I don't
have clear answers:
- How much of this device specific data is too much? If a driver
supports 10 different models, and the sequences for each model take 100
bytes, this 1000 bytes doesn't sound too much. But where's the limit?
And even if one driver only has 1kB of this data, what if we have lots
of similar drivers?
- How many bytes does a power sequence presented in this series take, if
the sequence contains, say, 5 steps, with gpio, delay, pwm, delay and
regulator?
- How likely is it that we'll get lots of different models? A hundred
different models for a backlight PWM with different power-on/off
sequences already sounds a really big number. If we're only going to
have each driver supporting, say, no more than 10 models, perhaps the
problem is not even an issue in practice.
- Are there ways to limit this bloat in the driver? One obvious way
would be to discard the unused sequences after driver probe, but that
only works for platform devices. Although, I guess these sequences would
mostly be used by platform drivers? If so, then the problem could be
solved by discarding the data after probe. Another would be to load the
sequences from a file as firmware, but that feels quite an awful
solution. Anybody have other ideas?
One clear positive side with in-driver approach is that it's totally
inside the kernel, and can be easily reworked in the future, or even
changed to a DT-based approach as presented in this series if that seems
like a better solution. The DT based approach, on the other hand, will
be more or less written to stone after it's merged.
So, I like the framework of expressing the sequences presented in this
series (except there's a problem with error cases, as I pointed out in
another post), as it can be used inside the drivers. But I'm not so
enthusiastic about presenting the sequences in DT.
My suggestion would be to go forward with an in-driver solution, and
look at the DT based solution later if we are seeing an increasing bloat
in the drivers.
Tomi
^ permalink raw reply
* [PULL REQUEST for Rafael] PM / devfreq: pull request-2 bugfixes
From: MyungJoo Ham @ 2012-11-21 10:45 UTC (permalink / raw)
To: rjw; +Cc: linux-pm, sachin.kamat, myungjoo.ham
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
The following changes since commit aa0745b51987fbaf628ee0943907db177335522d:
Merge branch 'pm-devfreq-next' into linux-next (2012-11-21 01:27:47 +0100)
, which is HEAD of Rafael's linux-pm.git linux-next branch.
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git tags/pull_req_20121121
MyungJoo Ham (2):
PM / devfreq: remove compiler error when a governor is module
PM / devfreq: missing rcu_read_lock() added for find_device_opp()
Sachin Kamat (2):
PM / devfreq: Fix incorrect argument in error message
PM / devfreq: Fix return value in devfreq_remove_governor()
drivers/devfreq/devfreq.c | 30 ++++++++++++++++++++++--------
include/linux/devfreq.h | 2 +-
2 files changed, 23 insertions(+), 9 deletions(-)
- ---
The four patches are trivial bugfixes following up the previous pull-request.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
iQIcBAEBAgAGBQJQrK/+AAoJEBOhurvWBoCKFL4P/2OVlI+s3IxG+XmhJbBY6h9N
PLvxP3pTT9o3IvITZ66ASX/ZwYL5HxX0Nq0gT1Z7kNxJ9O0m3o/3+JPPFS8b60GX
LGU/FE5SNgNAG9PsS9cq3CTd9EUUb1YVb6uj0pWNjgCvvGrwZ+UmQKJc/FSdzdiD
weFd6szc4M2chWl7M/EolsWzoZ3r79gOJPal0yw/9G1RbTCUSRnC1dxb1kAEcvmv
bqf4EBk6IToc1REdtQ8bn6ihHBkqHKFM1MygWF+L/Dwowwme9OwraHrg3SehwU4Q
lYlVPkbc1JcsxKhpdEs95Mtz0CFHdhskxo+SM1DpBZ2HpzDzti8gqV8MVyNzpQfw
WqKjFixIaMcAZ9YsKOgmQbBiGtwprTBn8a4OZZC25LNfEmkFHScEoHUYEsp9aakj
xfTgoKZ5qAxBvo/Xyl+aT3VUnyHvPk/TXai7AySBf0wTpk6YDYL2NpRY4dHCf7lL
MUxXQgXBJzIauFZyGZVGTfHBBsLFCkOcWdkymjMZ0usGTKUNT8cYHkbMf1VsLz9w
HGdhpuejhfoInuVQDesXkgmT7NcdBI0mkmIKRbPvAl7nVgRMbJMn13qKvDGzgvWd
VM2ZxZki/PNYf80+8m+oDnHvTvRJ8msy7lti8n6+OGFjlAueweU0B1TsS7Gp3Ths
cexSPLga49rnmf6UyXZ5
=wfji
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v3 RESEND 0/4] ARM: EXYNOS: Power domain DT support extension
From: Tomasz Figa @ 2012-11-21 10:38 UTC (permalink / raw)
To: linux-samsung-soc
Cc: linux-pm, linux-arm-kernel, kyungmin.park, kgene.kim,
thomas.abraham, rjw, m.szyprowski, tomasz.figa
In-Reply-To: <1352812107-8777-1-git-send-email-t.figa@samsung.com>
Hi Kgene,
On Tuesday 13 of November 2012 14:08:23 Tomasz Figa wrote:
> This patch series fixes two issues with existing DT support for Exynos
> power domains and extends it with the ability of binding devices to
> domains, basically making it possible to use power domains with DT.
>
> Based on for-next branch of Kgene's tree.
>
> Changes since v2:
> - Rebased to for-next of Kgene's tree.
> Changes since v1:
> - Added "samsung," prefix to "power-domain" property.
> - Added power domain nodes to Exynos4 device tree sources.
>
> Tomasz Figa (4):
> ARM: EXYNOS: pm_domain: Detect domain state on registration from DT
> ARM: EXYNOS: pm_domain: Fix power domain name initialization
> ARM: EXYNOS: pm_domain: Bind devices to power domains using DT
> ARM: dts: exynos4: Set up power domains
>
> .../bindings/arm/exynos/power_domain.txt | 15 +++-
> arch/arm/boot/dts/exynos4.dtsi | 30 +++++++
> arch/arm/boot/dts/exynos4210.dtsi | 5 ++
> arch/arm/mach-exynos/pm_domains.c | 93
> +++++++++++++++++++++- 4 files changed, 135 insertions(+), 8
> deletions(-)
Since this version is just a rebase to your for-next and there are no
remaining comments, could you still take it for 3.8?
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform
^ permalink raw reply
* [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
From: MyungJoo Ham @ 2012-11-21 10:10 UTC (permalink / raw)
To: linux-pm; +Cc: rjw, keescook, serge.hallyn, linux-kernel, myungjoo.ham
In-Reply-To: <CAGXu5j+g8inUZV43GFQzs9KpRvjPGPTJsXinb4Y=wOuOf2jnVQ@mail.gmail.com>
opp_get_notifier() uses find_device_opp(), which requires to
held rcu_read_lock. In order to keep the notifier-header
valid, we have added rcu_read_lock().
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
---
drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 45e053e..e91cb22 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1023,11 +1023,18 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
*/
int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
{
- struct srcu_notifier_head *nh = opp_get_notifier(dev);
+ struct srcu_notifier_head *nh;
+ int ret = 0;
+ rcu_read_lock();
+ nh = opp_get_notifier(dev);
if (IS_ERR(nh))
- return PTR_ERR(nh);
- return srcu_notifier_chain_register(nh, &devfreq->nb);
+ ret = PTR_ERR(nh);
+ if (!ret)
+ ret = srcu_notifier_chain_register(nh, &devfreq->nb);
+ rcu_read_unlock();
+
+ return ret;
}
/**
@@ -1042,11 +1049,18 @@ int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
*/
int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
{
- struct srcu_notifier_head *nh = opp_get_notifier(dev);
+ struct srcu_notifier_head *nh;
+ int ret = 0;
+ rcu_read_lock();
+ nh = opp_get_notifier(dev);
if (IS_ERR(nh))
- return PTR_ERR(nh);
- return srcu_notifier_chain_unregister(nh, &devfreq->nb);
+ ret = PTR_ERR(nh);
+ if (!ret)
+ ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
+ rcu_read_unlock();
+
+ return ret;
}
MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-11-21 10:00 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Anton Vorontsov, Stephen Warren, Thierry Reding, Mark Zhang,
Grant Likely, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann, linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-pm@vger.kernel.org,
Alexandre Courbot
In-Reply-To: <50AC956D.7020303@ti.com>
On Wednesday 21 November 2012 16:48:45 Tomi Valkeinen wrote:
> If the power-off sequence disables a regulator that was supposed to be
> enabled by the power-on sequence (but wasn't enabled because of an
> error), the regulator_disable is still called when the driver runs the
> power-off sequence, isn't it? Regulator enables and disables are ref
> counted, and the enables should match the disables.
And there collapses my theory.
> > Failures might be better handled if sequences have some "recovery policy"
> > about what to do when they fail, as mentioned in the link above. As you
> > pointed out, the driver might not always know enough about the resources
> > involved to do the right thing.
>
> Yes, I think such recovery policy would be needed.
Indeed, from your last paragraph this makes even more sense now.
Oh, and I noticed I forgot to reply to this:
> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
> device, no matter where and how many times it's used?
That's true - however when dereferencing the phandle, the underlying framework
will try to acquire the PWM, which will result in failure if the same resource
is referenced several times.
One could compare the phandles to avoid this, but in your example you must
know that for PWMs the "xyz" part is not relevant for comparison.
This makes referencing of resources by name much easier to implement and more
elegant with respect to frameworks leveraging.
Alex.
^ permalink raw reply
* Re: [PATCH v3 1/2] thermal: exynos: Fix wrong bit to control tmu core
From: Amit Kachhap @ 2012-11-21 9:46 UTC (permalink / raw)
To: Zhang Rui
Cc: Kyungmin Park, Jonghwan Choi, jonghwa3.lee, open list,
Sachin Kamat, Linux PM list, dg77.kim
In-Reply-To: <1353390792.3833.1.camel@rzhang1-mobl4>
On 20 November 2012 11:23, Zhang Rui <rui.zhang@intel.com> wrote:
> On Tue, 2012-11-20 at 10:39 +0900, Kyungmin Park wrote:
>> On 11/20/12, Jonghwan Choi <jhbird.choi@samsung.com> wrote:
>> > [0]bit is used to enable/disable tmu core. [1] bit is a reserved bit.
>> >
>> > Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> Amit and Donggeun Kim,
>
> any comments on this patch?
>
> thanks,
> rui
Hi Riu,
This patch is according to the suggestion I made earlier and looks fine.
Acked-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
Thanks,
Amit Daniel
>
>> > ---
>> > drivers/thermal/exynos_thermal.c | 16 ++++++++++++----
>> > 1 files changed, 12 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/thermal/exynos_thermal.c
>> > b/drivers/thermal/exynos_thermal.c
>> > index 6dd29e4..129e827 100644
>> > --- a/drivers/thermal/exynos_thermal.c
>> > +++ b/drivers/thermal/exynos_thermal.c
>> > @@ -52,9 +52,12 @@
>> >
>> > #define EXYNOS_TMU_TRIM_TEMP_MASK 0xff
>> > #define EXYNOS_TMU_GAIN_SHIFT 8
>> > +#define EXYNOS_TMU_GAIN_MASK (0xF << EXYNOS_TMU_GAIN_SHIFT)
>> > #define EXYNOS_TMU_REF_VOLTAGE_SHIFT 24
>> > -#define EXYNOS_TMU_CORE_ON 3
>> > -#define EXYNOS_TMU_CORE_OFF 2
>> > +#define EXYNOS_TMU_REF_VOLTAGE_MASK (0x1F <<
>> > EXYNOS_TMU_REF_VOLTAGE_SHIFT)
>> > +#define EXYNOS_TMU_CORE_ON BIT(0)
>> > +#define EXYNOS_TMU_CORE_ON_SHIFT 0
>> > +#define EXYNOS_TMU_CORE_ON_MASK (0x1 <<
>> > EXYNOS_TMU_CORE_ON_SHIFT)
>> > #define EXYNOS_TMU_DEF_CODE_TO_TEMP_OFFSET 50
>> >
>> > /* Exynos4210 specific registers */
>> > @@ -85,7 +88,9 @@
>> > #define EXYNOS_TMU_CLEAR_FALL_INT (0x111 << 16)
>> > #define EXYNOS_MUX_ADDR_VALUE 6
>> > #define EXYNOS_MUX_ADDR_SHIFT 20
>> > +#define EXYNOS_MUX_ADDR_MASK (0x7 << EXYNOS_MUX_ADDR_SHIFT)
>> > #define EXYNOS_TMU_TRIP_MODE_SHIFT 13
>> > +#define EXYNOS_TMU_TRIP_MODE_MASK (0x7 << EXYNOS_TMU_TRIP_MODE_SHIFT)
>> >
>> > #define EFUSE_MIN_VALUE 40
>> > #define EFUSE_MAX_VALUE 100
>> > @@ -650,10 +655,14 @@ static void exynos_tmu_control(struct platform_device
>> > *pdev, bool on)
>> > mutex_lock(&data->lock);
>> > clk_enable(data->clk);
>> >
>> > - con = pdata->reference_voltage << EXYNOS_TMU_REF_VOLTAGE_SHIFT |
>> > + con = readl(data->base + EXYNOS_TMU_REG_CONTROL);
>> > + con &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK | EXYNOS_TMU_GAIN_MASK |
>> > + EXYNOS_TMU_CORE_ON_MASK);
>> > + con |= pdata->reference_voltage << EXYNOS_TMU_REF_VOLTAGE_SHIFT |
>> > pdata->gain << EXYNOS_TMU_GAIN_SHIFT;
>> >
>> > if (data->soc == SOC_ARCH_EXYNOS) {
>> > + con &= ~(EXYNOS_TMU_TRIP_MODE_MASK | EXYNOS_MUX_ADDR_MASK);
>> > con |= pdata->noise_cancel_mode <<
>> > EXYNOS_TMU_TRIP_MODE_SHIFT;
>> > con |= (EXYNOS_MUX_ADDR_VALUE << EXYNOS_MUX_ADDR_SHIFT);
>> > }
>> > @@ -665,7 +674,6 @@ static void exynos_tmu_control(struct platform_device
>> > *pdev, bool on)
>> > pdata->trigger_level1_en << 4 |
>> > pdata->trigger_level0_en;
>> > } else {
>> > - con |= EXYNOS_TMU_CORE_OFF;
>> > interrupt_en = 0; /* Disable all interrupts */
>> > }
>> > writel(interrupt_en, data->base + EXYNOS_TMU_REG_INTEN);
>> > --
>> > 1.7.4.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at http://www.tux.org/lkml/
>> >
>
>
^ permalink raw reply
* Re: [PATCH 2/2] Thermal: Introduce THERMAL_TREND_RAISE/DROP_FULL support for step_wise governor
From: Amit Kachhap @ 2012-11-21 9:37 UTC (permalink / raw)
To: Zhang Rui; +Cc: Linux PM list, durga
In-Reply-To: <1353313293.6468.4.camel@rzhang1-mobl4>
On 19 November 2012 13:51, Zhang Rui <rui.zhang@intel.com> wrote:
> From fcc7e0a36388f468c25526290e2fb2beebaae99c Mon Sep 17 00:00:00 2001
> From: Zhang Rui <rui.zhang@intel.com>
> Date: Mon, 19 Nov 2012 16:10:20 +0800
> Subject: [PATCH 2/2] Introduce THERMAL_TREND_RAISE/DROP_FULL support for
> step_wise governor
>
> step_wise governor should set the device cooling state to
> upper/lower limit directly when THERMAL_TREND_RAISE/DROP_FULL.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
> drivers/thermal/step_wise.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/thermal/step_wise.c b/drivers/thermal/step_wise.c
> index 1242cff..6273f7d 100644
> --- a/drivers/thermal/step_wise.c
> +++ b/drivers/thermal/step_wise.c
> @@ -35,6 +35,10 @@
> * state for this trip point
> * b. if the trend is THERMAL_TREND_DROPPING, use lower cooling
> * state for this trip point
> + * c. if the trend is THERMAL_TREND_RAISE_FULL, use upper limit
> + * for this trip point
> + * d. if the trend is THERMAL_TREND_DROP_FULL, use lower limit
> + * for this trip point
> */
> static unsigned long get_target_state(struct thermal_instance *instance,
> enum thermal_trend trend)
> @@ -44,12 +48,23 @@ static unsigned long get_target_state(struct thermal_instance *instance,
>
> cdev->ops->get_cur_state(cdev, &cur_state);
>
> - if (trend == THERMAL_TREND_RAISING) {
> + switch (trend) {
> + case THERMAL_TREND_RAISING:
> cur_state = cur_state < instance->upper ?
> (cur_state + 1) : instance->upper;
> - } else if (trend == THERMAL_TREND_DROPPING) {
> + break;
> + case THERMAL_TREND_DROPPING:
> cur_state = cur_state > instance->lower ?
> (cur_state - 1) : instance->lower;
> + break;
> + case THERMAL_TREND_RAISE_FULL:
> + cur_state = instance->upper;
> + break;
> + case THERMAL_TREND_DROP_FULL:
> + cur_state = instance->lower;
> + break;
> + default:
> + break;
> }
>Hi Rui,
Sorry for late review. This part looks good. But can this
get_target_state be also called in update_instance_for_dethrottle as
done by my patch for quick cooling.
Thanks,
Amit Daniel
> return cur_state;
> --
> 1.7.9.5
>
>
>
^ permalink raw reply
* RE: [PATCH 0/4] thermal: Add support for interrupt based notification to thermal layer
From: Zhang, Rui @ 2012-11-21 9:04 UTC (permalink / raw)
To: Amit Daniel Kachhap, linux-pm@lists.linux-foundation.org
Cc: linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
R, Durgadoss, lenb@kernel.org, linux-acpi@vger.kernel.org,
jonghwa3.lee@samsung.com, linux-pm@vger.kernel.org
In-Reply-To: <1352348786-24255-1-git-send-email-amit.kachhap@linaro.org>
Hi, Amit,
As THERMAL_TREND_RAISE_FULL/THERMAL_TREND_DROP_FULL
has been introduced to thermal next tree,
I'd like to get your plan about this patch set?
Thanks,
Rui
> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Amit Daniel Kachhap
> Sent: Thursday, November 08, 2012 12:26 PM
> To: linux-pm@lists.linux-foundation.org
> Cc: linux-samsung-soc@vger.kernel.org; linux-kernel@vger.kernel.org; R,
> Durgadoss; lenb@kernel.org; Zhang, Rui; linux-acpi@vger.kernel.org;
> amit.kachhap@linaro.org; jonghwa3.lee@samsung.com
> Subject: [PATCH 0/4] thermal: Add support for interrupt based
> notification to thermal layer
> Importance: High
>
> The patch submitted by Jonghwa Lee
> (https://patchwork.kernel.org/patch/1683441/)
> adds support for interrupt based notification to thermal layer. This is
> a good feature but the current thermal framework needs polling/regular
> notification for invoking suitable cooling action. So adding 2 new
> thermal trend type to implement this feature.
>
> All these patches are based on thermal maintainer next tree.
> git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux.git next
>
> Amit Daniel Kachhap (3):
> thermal: Add new thermal trend type to support quick cooling
> thermal: exynos: Miscellaneous fixes to support falling threshold
> interrupt
> thermal: exynos: Use the new thermal trend type for quick cooling
> action.
>
> Jonghwa Lee (1):
> Thermal: exynos: Add support for temperature falling interrupt.
>
> drivers/thermal/exynos_thermal.c | 105 +++++++++++++++---
> --------
> drivers/thermal/step_wise.c | 19 ++++-
> include/linux/platform_data/exynos_thermal.h | 3 +
> include/linux/thermal.h | 2 +
> 4 files changed, 80 insertions(+), 49 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
> in the body of a message to majordomo@vger.kernel.org More majordomo
> info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RFC PATCH] PM / devfreq: Add runtime-pm support
From: Rajagopal Venkat @ 2012-11-21 9:01 UTC (permalink / raw)
To: myungjoo.ham-Sze3O3UU22JBDgjK7y7TUQ, rjw-KKrjLPT3xs0,
nm-l0cyMroinI0, mturquette-QSEj5FYQhm4dnm+yROfE0A,
kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ
Cc: linux-pm-u79uwXL29TY76Z2rM5mHXA,
linaro-dev-cunTk1MwBs8s++Sfvej+rw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
patches-QSEj5FYQhm4dnm+yROfE0A
Instead of devfreq device driver explicitly calling devfreq suspend
and resume apis perhaps from runtime-pm suspend and resume callbacks,
let devfreq core handle it automatically.
Attach devfreq core to runtime-pm framework so that, devfreq device
driver pm_runtime_suspend() will automatically suspend the devfreq
and pm_runtime_resume() will resume the devfreq.
Signed-off-by: Rajagopal Venkat <rajagopal.venkat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
drivers/devfreq/devfreq.c | 145 ++++++++++++++++++++++++++++++++--------------
include/linux/devfreq.h | 12 ----
2 files changed, 102 insertions(+), 55 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 45e053e..190e414 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -25,10 +25,9 @@
#include <linux/list.h>
#include <linux/printk.h>
#include <linux/hrtimer.h>
+#include <linux/pm_runtime.h>
#include "governor.h"
-static struct class *devfreq_class;
-
/*
* devfreq core provides delayed work based load monitoring helper
* functions. Governors can use these or can implement their own
@@ -414,6 +413,93 @@ static void devfreq_dev_release(struct device *dev)
}
/**
+ * devfreq_suspend_device() - Suspend devfreq of a device.
+ * @devfreq: the devfreq instance to be suspended
+ */
+static int devfreq_suspend_device(struct devfreq *devfreq)
+{
+ if (!devfreq)
+ return -EINVAL;
+
+ if (!devfreq->governor)
+ return 0;
+
+ return devfreq->governor->event_handler(devfreq,
+ DEVFREQ_GOV_SUSPEND, NULL);
+}
+
+/**
+ * devfreq_resume_device() - Resume devfreq of a device.
+ * @devfreq: the devfreq instance to be resumed
+ */
+static int devfreq_resume_device(struct devfreq *devfreq)
+{
+ if (!devfreq)
+ return -EINVAL;
+
+ if (!devfreq->governor)
+ return 0;
+
+ return devfreq->governor->event_handler(devfreq,
+ DEVFREQ_GOV_RESUME, NULL);
+}
+
+static int devfreq_runtime_suspend(struct device *dev)
+{
+ int ret;
+ struct devfreq *devfreq;
+
+ mutex_lock(&devfreq_list_lock);
+ devfreq = find_device_devfreq(dev);
+ mutex_unlock(&devfreq_list_lock);
+
+ ret = devfreq_suspend_device(devfreq);
+ if (ret < 0)
+ goto out;
+
+ ret = pm_generic_runtime_suspend(dev);
+out:
+ return ret;
+}
+
+static int devfreq_runtime_resume(struct device *dev)
+{
+ int ret;
+ struct devfreq *devfreq;
+
+ mutex_lock(&devfreq_list_lock);
+ devfreq = find_device_devfreq(dev);
+ mutex_unlock(&devfreq_list_lock);
+
+ ret = devfreq_resume_device(devfreq);
+ if (ret < 0)
+ goto out;
+
+ ret = pm_generic_runtime_resume(dev);
+out:
+ return ret;
+}
+
+static int devfreq_runtime_idle(struct device *dev)
+{
+ return pm_generic_runtime_idle(dev);
+}
+
+static const struct dev_pm_ops devfreq_pm_ops = {
+ SET_RUNTIME_PM_OPS(
+ devfreq_runtime_suspend,
+ devfreq_runtime_resume,
+ devfreq_runtime_idle
+ )
+};
+
+static struct class devfreq_class = {
+ .name = "devfreq",
+ .owner = THIS_MODULE,
+ .pm = &devfreq_pm_ops,
+};
+
+/**
* devfreq_add_device() - Add devfreq feature to the device
* @dev: the device to add devfreq feature.
* @profile: device-specific profile to run devfreq.
@@ -454,8 +540,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
mutex_init(&devfreq->lock);
mutex_lock(&devfreq->lock);
+ dev->class = &devfreq_class;
devfreq->dev.parent = dev;
- devfreq->dev.class = devfreq_class;
+ devfreq->dev.class = &devfreq_class;
devfreq->dev.release = devfreq_dev_release;
devfreq->profile = profile;
strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
@@ -498,6 +585,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
goto err_init;
}
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_active(dev);
+
return devfreq;
err_init:
@@ -526,40 +616,6 @@ int devfreq_remove_device(struct devfreq *devfreq)
EXPORT_SYMBOL(devfreq_remove_device);
/**
- * devfreq_suspend_device() - Suspend devfreq of a device.
- * @devfreq: the devfreq instance to be suspended
- */
-int devfreq_suspend_device(struct devfreq *devfreq)
-{
- if (!devfreq)
- return -EINVAL;
-
- if (!devfreq->governor)
- return 0;
-
- return devfreq->governor->event_handler(devfreq,
- DEVFREQ_GOV_SUSPEND, NULL);
-}
-EXPORT_SYMBOL(devfreq_suspend_device);
-
-/**
- * devfreq_resume_device() - Resume devfreq of a device.
- * @devfreq: the devfreq instance to be resumed
- */
-int devfreq_resume_device(struct devfreq *devfreq)
-{
- if (!devfreq)
- return -EINVAL;
-
- if (!devfreq->governor)
- return 0;
-
- return devfreq->governor->event_handler(devfreq,
- DEVFREQ_GOV_RESUME, NULL);
-}
-EXPORT_SYMBOL(devfreq_resume_device);
-
-/**
* devfreq_add_governor() - Add devfreq governor
* @governor: the devfreq governor to be added
*/
@@ -730,6 +786,7 @@ out:
ret = count;
return ret;
}
+
static ssize_t show_available_governors(struct device *d,
struct device_attribute *attr,
char *buf)
@@ -952,19 +1009,21 @@ static struct device_attribute devfreq_attrs[] = {
static int __init devfreq_init(void)
{
- devfreq_class = class_create(THIS_MODULE, "devfreq");
- if (IS_ERR(devfreq_class)) {
+ int err;
+
+ err = class_register(&devfreq_class);
+ if (err < 0) {
pr_err("%s: couldn't create class\n", __FILE__);
- return PTR_ERR(devfreq_class);
+ return err;
}
devfreq_wq = create_freezable_workqueue("devfreq_wq");
if (IS_ERR(devfreq_wq)) {
- class_destroy(devfreq_class);
+ class_unregister(&devfreq_class);
pr_err("%s: couldn't create workqueue\n", __FILE__);
return PTR_ERR(devfreq_wq);
}
- devfreq_class->dev_attrs = devfreq_attrs;
+ devfreq_class.dev_attrs = devfreq_attrs;
return 0;
}
@@ -972,7 +1031,7 @@ subsys_initcall(devfreq_init);
static void __exit devfreq_exit(void)
{
- class_destroy(devfreq_class);
+ class_unregister(&devfreq_class);
destroy_workqueue(devfreq_wq);
}
module_exit(devfreq_exit);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 235248c..4fd920f 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -181,8 +181,6 @@ extern struct devfreq *devfreq_add_device(struct device *dev,
const char *governor_name,
void *data);
extern int devfreq_remove_device(struct devfreq *devfreq);
-extern int devfreq_suspend_device(struct devfreq *devfreq);
-extern int devfreq_resume_device(struct devfreq *devfreq);
/* Helper functions for devfreq user device driver with OPP. */
extern struct opp *devfreq_recommended_opp(struct device *dev,
@@ -226,16 +224,6 @@ static int devfreq_remove_device(struct devfreq *devfreq)
return 0;
}
-static int devfreq_suspend_device(struct devfreq *devfreq)
-{
- return 0;
-}
-
-static int devfreq_resume_device(struct devfreq *devfreq)
-{
- return 0;
-}
-
static struct opp *devfreq_recommended_opp(struct device *dev,
unsigned long *freq, u32 flags)
{
--
1.7.11.3
^ permalink raw reply related
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 8:48 UTC (permalink / raw)
To: Alex Courbot
Cc: Anton Vorontsov, Stephen Warren, Thierry Reding, Mark Zhang,
Grant Likely, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot
In-Reply-To: <1699753.ZQsWMHINxd@percival>
[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]
On 2012-11-21 10:32, Alex Courbot wrote:
>> Ok. I'll need to dig up the conversation
>
> IIRC it was somewhere around here:
>
> https://lkml.org/lkml/2012/9/7/662
>
> See the parent messages too.
Thanks.
>> Did you consider any examples
>> of how some driver could handle the error cases?
>
> For all the (limited) use cases I considered, playing the power-off sequence
> when power-on fails just works. If power-off also fails you are potentially in
> more trouble though. Maybe we could have another "run" function that does not
> stop on errors for handling such cases where you want to "stop everything you
> can".
If the power-off sequence disables a regulator that was supposed to be
enabled by the power-on sequence (but wasn't enabled because of an
error), the regulator_disable is still called when the driver runs the
power-off sequence, isn't it? Regulator enables and disables are ref
counted, and the enables should match the disables.
> Failures might be better handled if sequences have some "recovery policy"
> about what to do when they fail, as mentioned in the link above. As you
> pointed out, the driver might not always know enough about the resources
> involved to do the right thing.
Yes, I think such recovery policy would be needed.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-11-21 8:32 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Anton Vorontsov, Stephen Warren, Thierry Reding, Mark Zhang,
Grant Likely, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot
In-Reply-To: <50AC8D3B.6040300-l0cyMroinI0@public.gmane.org>
On Wednesday 21 November 2012 16:13:47 Tomi Valkeinen wrote:
> * PGP Signed by an unknown key
>
> On 2012-11-21 03:56, Alex Courbot wrote:
> > Hi Tomi,
> >
> > On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote:
> >> I guess there's a reason, but the above looks a bit inconsistent. For
> >> gpio you define the gpio resource inside the step. For power and pwm the
> >> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
> >> 5000000>;" work in step2?
> >
> > That's mostly a framework issue. Most frameworks do not export a function
> > that allow to dereference a phandle - they expect resources to be
> > declared right under the device node and accessed by name through
> > foo_get(device, name). So using phandles in power sequences would require
> > to export these additional
> Right, I expected something like that.
>
> > functions and also opens the door to some inconsistencies - for instance,
> > your PWM phandle could be referenced a second time in the sequence with a
> > different period - how do you know that these are actually referring the
> > same PWM device?
>
> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
> device, no matter where and how many times it's used?
>
> >>> +When a power sequence is run, its steps is executed one after the other
> >>> until +one step fails or the end of the sequence is reached.
> >>
> >> The document doesn't give any hint of what the driver should do if
> >> running the power sequence fails. Run the "opposite" power sequence?
> >> Will that work for all resources? I'm mainly thinking of a case where
> >> each enable of the resource should be matched by a disable, i.e. you
> >> can't call disable if no enable was called.
> >
> > We discussed that issue already (around v5 I think) and the conclusion was
> > that it should be up to the driver. When we simply enable/disable
> > resources it is easy to revert, but in the future non-boolean properties
> > will likely be introduced, and these cannot easily be reverted. Moreover
> > some drivers might have more complex recovery needs. This deserves more
> > discussion I think, as I'd like to have some "generic" recovery mechanism
> > that covers most of the cases.
>
> Ok. I'll need to dig up the conversation
IIRC it was somewhere around here:
https://lkml.org/lkml/2012/9/7/662
See the parent messages too.
> Did you consider any examples
> of how some driver could handle the error cases?
For all the (limited) use cases I considered, playing the power-off sequence
when power-on fails just works. If power-off also fails you are potentially in
more trouble though. Maybe we could have another "run" function that does not
stop on errors for handling such cases where you want to "stop everything you
can".
> What I'm worried about is that, as far as I understand, the power
> sequence is kinda like black box to the driver. The driver just does
> "power-up", without knowing what really goes on in there.
The driver could always inspect the sequence, but you are right in that this
is not how it is intended to be done.
> And if it doesn't know what goes on in there, nor what's in "power-down"
> sequence, how can it do anything when an error happens? The only option
> I see is that the driver doesn't do anything, which will leave some
> resources enabled, or it can run the power-down sequence, which may or
> may not work.
Failures might be better handled if sequences have some "recovery policy"
about what to do when they fail, as mentioned in the link above. As you
pointed out, the driver might not always know enough about the resources
involved to do the right thing.
Alex.
^ permalink raw reply
* [PATCH] PM / devfreq: remove compiler error when a governor is module
From: MyungJoo Ham @ 2012-11-21 8:25 UTC (permalink / raw)
To: linux-pm; +Cc: rjw, nm, myungjoo.ham
With the intruction of patch, eff607fdb1f787da1fedf46ab6e64adc2afd1c5a,
it became possible to include a governor as a module.
Thus the #ifdef statement for a governor should become #if IS_ENABLED.
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
---
include/linux/devfreq.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 235248c..e83ef39 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -192,7 +192,7 @@ extern int devfreq_register_opp_notifier(struct device *dev,
extern int devfreq_unregister_opp_notifier(struct device *dev,
struct devfreq *devfreq);
-#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
+#if IS_ENABLED(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND)
/**
* struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
* and devfreq_add_device
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 8:13 UTC (permalink / raw)
To: Alex Courbot
Cc: Anton Vorontsov, Stephen Warren, Thierry Reding, Mark Zhang,
Grant Likely, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot
In-Reply-To: <4316169.5QXVzv7peZ@percival>
[-- Attachment #1: Type: text/plain, Size: 2745 bytes --]
On 2012-11-21 03:56, Alex Courbot wrote:
> Hi Tomi,
>
> On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote:
>> I guess there's a reason, but the above looks a bit inconsistent. For
>> gpio you define the gpio resource inside the step. For power and pwm the
>> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
>> 5000000>;" work in step2?
>
> That's mostly a framework issue. Most frameworks do not export a function that
> allow to dereference a phandle - they expect resources to be declared right
> under the device node and accessed by name through foo_get(device, name). So
> using phandles in power sequences would require to export these additional
Right, I expected something like that.
> functions and also opens the door to some inconsistencies - for instance, your
> PWM phandle could be referenced a second time in the sequence with a different
> period - how do you know that these are actually referring the same PWM
> device?
This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
device, no matter where and how many times it's used?
>>> +When a power sequence is run, its steps is executed one after the other
>>> until +one step fails or the end of the sequence is reached.
>>
>> The document doesn't give any hint of what the driver should do if
>> running the power sequence fails. Run the "opposite" power sequence?
>> Will that work for all resources? I'm mainly thinking of a case where
>> each enable of the resource should be matched by a disable, i.e. you
>> can't call disable if no enable was called.
>
> We discussed that issue already (around v5 I think) and the conclusion was
> that it should be up to the driver. When we simply enable/disable resources it
> is easy to revert, but in the future non-boolean properties will likely be
> introduced, and these cannot easily be reverted. Moreover some drivers might
> have more complex recovery needs. This deserves more discussion I think, as
> I'd like to have some "generic" recovery mechanism that covers most of the
> cases.
Ok. I'll need to dig up the conversation. Did you consider any examples
of how some driver could handle the error cases?
What I'm worried about is that, as far as I understand, the power
sequence is kinda like black box to the driver. The driver just does
"power-up", without knowing what really goes on in there.
And if it doesn't know what goes on in there, nor what's in "power-down"
sequence, how can it do anything when an error happens? The only option
I see is that the driver doesn't do anything, which will leave some
resources enabled, or it can run the power-down sequence, which may or
may not work.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* [PATCH v5] Thermal: exynos: Add sysfs node supporting exynos's emulation mode.
From: Jonghwa Lee @ 2012-11-21 4:31 UTC (permalink / raw)
To: linux-pm
Cc: linux-kernel, Len Brown, Durgadoss R, Rafael J. Wysocki,
Amit Dinel Kachhap, MyungJoo Ham, Kyungmin Park, Jonghwa Lee
This patch supports exynos's emulation mode with newly created sysfs node.
Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal
management unit. Thermal emulation mode supports software debug for TMU's
operation. User can set temperature manually with software code and TMU
will read current temperature from user value not from sensor's value.
This patch includes also documentary placed under Documentation/thermal/.
Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
---
v5
- Rebase the patch at -next branch of zhang rui's git.
- Fix EXYNOS_EMULATION_MODE Kconfig option as Amit's comment.
- Show emulation temperature in millicelsius.
Storing emulation temperature supports both celsius and mcelsius for input.
v4
- Fix Typo.
- Remove unnecessary codes.
- Add comments about feature of exynos emulation operation to the document.
v3
- Remove unnecessay variables.
- Do some code clean in exynos_tmu_emulation_store().
- Make wrapping function of sysfs node creation function to use
#ifdefs in minimum.
v2
exynos_thermal.c
- Fix build error occured by wrong emulation control register name.
- Remove exynos5410 dependent codes.
exynos_thermal_emulation
- Align indentation.
Documentation/thermal/exynos_thermal_emulation | 56 +++++++++++++
drivers/thermal/Kconfig | 9 ++
drivers/thermal/exynos_thermal.c | 103 ++++++++++++++++++++++++
3 files changed, 168 insertions(+), 0 deletions(-)
create mode 100644 Documentation/thermal/exynos_thermal_emulation
diff --git a/Documentation/thermal/exynos_thermal_emulation b/Documentation/thermal/exynos_thermal_emulation
new file mode 100644
index 0000000..bc9b057
--- /dev/null
+++ b/Documentation/thermal/exynos_thermal_emulation
@@ -0,0 +1,56 @@
+EXYNOS EMULATION MODE
+========================
+
+Copyright (C) 2012 Samsung Electronics
+
+Written by Jonghwa Lee <jonghwa3.lee@samsung.com>
+
+Description
+-----------
+
+Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal management unit.
+Thermal emulation mode supports software debug for TMU's operation. User can set temperature
+manually with software code and TMU will read current temperature from user value not from
+sensor's value.
+
+Enabling CONFIG_EXYNOS_THERMAL_EMUL option will make this support in available.
+When it's enabled, sysfs node will be created under
+/sys/bus/platform/devices/'exynos device name'/ with name of 'emulation'.
+
+The sysfs node, 'emulation', will contain value 0 for the initial state. When you input any
+temperature you want to update to sysfs node, it automatically enable emulation mode and
+current temperature will be changed into it.
+(Exynos also supports user changable delay time which would be used to delay of
+ changing temperature. However, this node only uses same delay of real sensing time, 938us.)
+
+Exynos emulation mode requires synchronous of value changing and enabling. It means when you
+want to update the any value of delay or next temperature, then you have to enable emulation
+mode at the same time. (Or you have to keep the mode enabling.) If you don't, it fails to
+change the value to updated one and just use last succeessful value repeatedly. That's why
+this node gives users the right to change termerpature only. Just one interface makes it more
+simply to use.
+
+Disabling emulation mode only requires writing value 0 to sysfs node.
+
+
+TEMP 120 |
+ |
+ 100 |
+ |
+ 80 |
+ | +-----------
+ 60 | | |
+ | +-------------| |
+ 40 | | | |
+ | | | |
+ 20 | | | +----------
+ | | | | |
+ 0 |______________|_____________|__________|__________|_________
+ A A A A TIME
+ |<----->| |<----->| |<----->| |
+ | 938us | | | | | |
+emulation : 0 50 | 70 | 20 | 0
+current temp : sensor 50 70 20 sensor
+
+
+
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index d96da07..61ab206 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -101,6 +101,15 @@ config EXYNOS_THERMAL
If you say yes here you get support for TMU (Thermal Managment
Unit) on SAMSUNG EXYNOS series of SoC.
+config EXYNOS_THERMAL_EMUL
+ bool "EXYNOS TMU emulation mode support"
+ depends on EXYNOS_THERMAL
+ help
+ Exynos 4412 and 4414 and 5 series has emulation mode on TMU.
+ Enable this option will be make sysfs node in exynos thermal platform
+ device directory to support emulation mode. With emulation mode sysfs
+ node, you can manually input temperature to TMU for simulation purpose.
+
config DB8500_THERMAL
bool "DB8500 thermal management"
depends on ARCH_U8500
diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
index 7772d16..75615f2 100644
--- a/drivers/thermal/exynos_thermal.c
+++ b/drivers/thermal/exynos_thermal.c
@@ -99,6 +99,14 @@
#define IDLE_INTERVAL 10000
#define MCELSIUS 1000
+#ifdef CONFIG_EXYNOS_THERMAL_EMUL
+#define EXYNOS_EMUL_TIME 0x57F0
+#define EXYNOS_EMUL_TIME_SHIFT 16
+#define EXYNOS_EMUL_DATA_SHIFT 8
+#define EXYNOS_EMUL_DATA_MASK 0xFF
+#define EXYNOS_EMUL_ENABLE 0x1
+#endif /* CONFIG_EXYNOS_THERMAL_EMUL */
+
/* CPU Zone information */
#define PANIC_ZONE 4
#define WARN_ZONE 3
@@ -832,6 +840,94 @@ static inline struct exynos_tmu_platform_data *exynos_get_driver_data(
return (struct exynos_tmu_platform_data *)
platform_get_device_id(pdev)->driver_data;
}
+
+#ifdef CONFIG_EXYNOS_THERMAL_EMUL
+static ssize_t exynos_tmu_emulation_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
+ struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+ unsigned int reg;
+ u8 temp_code;
+ int temp = 0;
+
+ if (data->soc == SOC_ARCH_EXYNOS4210)
+ goto out;
+
+ mutex_lock(&data->lock);
+ clk_enable(data->clk);
+ reg = readl(data->base + EXYNOS_EMUL_CON);
+ clk_disable(data->clk);
+ mutex_unlock(&data->lock);
+
+ if (reg & EXYNOS_EMUL_ENABLE) {
+ reg >>= EXYNOS_EMUL_DATA_SHIFT;
+ temp_code = reg & EXYNOS_EMUL_DATA_MASK;
+ temp = code_to_temp(data, temp_code);
+ }
+out:
+ return sprintf(buf, "%d\n", temp * MCELSIUS);
+}
+
+static ssize_t exynos_tmu_emulation_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
+ struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+ unsigned int reg;
+ int temp;
+
+ if (data->soc == SOC_ARCH_EXYNOS4210)
+ goto out;
+
+ if (!sscanf(buf, "%d\n", &temp) || temp < 0)
+ return -EINVAL;
+
+ mutex_lock(&data->lock);
+ clk_enable(data->clk);
+
+ reg = readl(data->base + EXYNOS_EMUL_CON);
+
+ if (temp) {
+ /* Both CELSIUS and MCELSIUS type are available for input */
+ if (temp > MCELSIUS)
+ temp /= MCELSIUS;
+
+ reg = (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT) |
+ (temp_to_code(data, (temp / MCELSIUS))
+ << EXYNOS_EMUL_DATA_SHIFT) | EXYNOS_EMUL_ENABLE;
+ } else {
+ reg &= ~EXYNOS_EMUL_ENABLE;
+ }
+
+ writel(reg, data->base + EXYNOS_EMUL_CON);
+
+ clk_disable(data->clk);
+ mutex_unlock(&data->lock);
+
+out:
+ return count;
+}
+
+static DEVICE_ATTR(emulation, 0644, exynos_tmu_emulation_show,
+ exynos_tmu_emulation_store);
+static int create_emulation_sysfs(struct device *dev)
+{
+ return device_create_file(dev, &dev_attr_emulation);
+}
+static void remove_emulation_sysfs(struct device *dev)
+{
+ device_remove_file(dev, &dev_attr_emulation);
+}
+#else
+static inline int create_emulation_sysfs(struct device *dev) {return 0;}
+static inline void remove_emulation_sysfs(struct device *dev){}
+#endif
+
static int __devinit exynos_tmu_probe(struct platform_device *pdev)
{
struct exynos_tmu_data *data;
@@ -930,6 +1026,11 @@ static int __devinit exynos_tmu_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "Failed to register thermal interface\n");
goto err_clk;
}
+
+ ret = create_emulation_sysfs(&pdev->dev);
+ if (ret)
+ dev_err(&pdev->dev, "Failed to create emulation mode sysfs node\n");
+
return 0;
err_clk:
platform_set_drvdata(pdev, NULL);
@@ -941,6 +1042,8 @@ static int __devexit exynos_tmu_remove(struct platform_device *pdev)
{
struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+ remove_emulation_sysfs(&pdev->dev);
+
exynos_tmu_control(pdev, false);
exynos_unregister_thermal();
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-11-21 4:23 UTC (permalink / raw)
To: Grant Likely
Cc: Anton Vorontsov, Stephen Warren, Thierry Reding, Mark Zhang,
Rob Herring, Mark Brown, David Woodhouse, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-pm@vger.kernel.org,
Alexandre Courbot
In-Reply-To: <20121120215429.B621F3E1821@localhost>
Hi Grant,
On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> > With the advent of the device tree and of ARM kernels that are not
> > board-tied, we cannot rely on these board-specific hooks anymore but
>
> This isn't strictly true. It is still perfectly fine to have board
> specific code when necessary. However, there is strong encouragement to
> enable that code in device drivers as much as possible and new board
> files need to have very strong justification.
But doesn't introducing board-specific code into the kernel just defeats the
purpose of the DT? If we extend this logic, we are heading straight back to
board-definition files. To a lesser extent than before I agree, but the problem
is fundamentally the same.
> > need a way to implement these sequences in a portable manner. This patch
> > introduces a simple interpreter that can execute such power sequences
> > encoded either as platform data or within the device tree. It also
> > introduces first support for regulator, GPIO and PWM resources.
>
> This is where I start getting nervous. Up to now I've strongly resisted
> adding any kind of interpreted code to the device tree. The model is to
> identify hardware, but require the driver to know how to control it. (as
> compared to ACPI which is entirely designed around executable
> bytecode).
>
> While the power sequences described here certainly cannot be confused
> with a Turing complete bytecode, it is a step in that direction.
Technically speaking power sequences are a step towards an interpreter, but it
is a very small one and it should not go much further than the current state.
I understand the concern of having "code" into the DT but I really think it
should be viewed from a different angle.
Powering sequences are special in that they can be affected by the board design
or the devices variations. For instance hundreds of different panels with
backlights are currently compatible with the pwm-backlight driver. The only
thing that differenciates them is how the backlight is powered on and off. If
you are to build a kernel that is supposed to support all these panels, you
would need to embed all the powering sequences in the kernel even though only
one of them will be used by one specific board. Power sequences in the DT help
preventing that.
With that stated, it is clear that we should not need to define more than the
short, simple sequences of actions that cannot be elegantly handled by the
driver. Anything beyond that should be handled by the driver itself. In
particular, here are a few things I do *not* want to see included in power
seqs:
- conditionals/jumps (or it's not a sequence anymore).
- direct access to hardware. Resources must at least be abstracted in some
way. You shall not e.g. access the address space directly.
- support for non-power related resources - that is out of the special case of
powering sequences and should be done by the driver
That should keep the "grammar" simple, and the sequences short enough to that
we can consider then as data belonging to the device, and not as code that is
interpreted.
> I think this will get very verbose in a hurry. Already this simple
> example is 45 lines long. Using the device tree structure to encode the
> language doesn't look like a very good fit. Not to mention that the
> order of operations is entirely based on the node name. Want to insert
> an operation between step0 and step1? Need to rename step1, step2, and
> step3 to do so.
I don't like that steps numbering thing neither, but it seems to be the best
way to do it so far.
As for the DT structure not being adapted for this - I would agree if we
wanted to implement a complete interpreter, but that's precisely not the case.
More about this later.
> This implementation also isn't very consistent. The gpio is referenced
> with a phandle in step3/step0, but the regulator and pwm are referenced
> by id.
Tomi made the same remark - the reason for using the phandle in GPIO is
because GPIO framework does not support referencing GPIOs by name yet. I
wanted to DT bindings to reflect the underlying framework as much as possible
until we have a function like gpio_get(device, id).
However I agree that this makes things inconsistent at the moment and would
require a bindings change. And in the case of the DT this is actually easy to
implement (I did it in some previous versions). I'll make sure to do it.
> As an alternative, what about something like the following?
>
> backlight {
> compatible = "pwm-backlight";
> ...
>
> /* resources used by the power sequences */
> pwms = <&pwm 2 5000000>;
> pwm-names = "backlight";
> regulators = <&backlight_reg>;
> gpios = <&gpio 28 0>;
>
> power-on-sequence = "r0e;d10000m;p0e;g0s";
> power-off-sequence = "g0c;p0d;d10000m;r0d";
> };
Well, *now* it really looks like bytecode. :)
> I'm thinking about the scripts as trivial-to-parse ascii strings that
> have a very simple set of commands. The commands use resources already
> defined in the node. ie. 'g0' refers to the first entry in the gpios
> property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> no means take this as the format to use, it is just an example off the
> top of my head, but it is already way easier to work with than putting
> each command into a node.
I'd argue that this opens the door wide open towards having a complete
interpreter in the device tree. At least DT nodes restrict what we can do
conveniently.
> The trick is still to define a syntax that doesn't fall apart when it
> needs to be extended. I would also like to get opinions on whether or
> not conditionals or loops should be supported (ie. loop waiting for a
> status to change). If they should then we need to be a lot more careful
> about the design (and due to my aforementioned nervousness, somebody may
> need to get me therapy).
That's IMHO the main argument in favor of using DT nodes here (the syntax
extension, not your therapy). They can be extended simply by adding properties
to them, and I was relying on this to make power seqs extensible while keeping
things consistent (and backward-compatible). For instance, when we want to
support voltage setting in regulators we can just add a "voltage" property for
that.
So to sum up the pros of the current node-base representation:
- give a "data like" representation of powering sequences (what they should
be, actually)
- puts sanity barriers for not turning power seqs into a real code interpreter
- easily extensible
There are probably a few cons, the numbering of steps being one, but at least
we don't need to design a new DSL and care too much about extensibility which
is, in the nodes case, built-in and free.
I also like to make it clear that I don't want to see this going out of
control and that any extension proposal would have to be thoroughly justified
and reviewed - and better not contradict any of the 3 points listed above.
If that makes you feel better, maybe we can try and fix what is wrong with the
current bindings instead of introducing a new language that will be, by
nature, more complex to handle and difficult to extend without breaking things?
Alex.
^ permalink raw reply
* Re: [PATCH 1/1] thermal: cpu cooling: use const parameter while registering
From: Zhang Rui @ 2012-11-21 2:51 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: amit.kachhap, linux-acpi, linux-pm, Linux PM list
In-Reply-To: <1352735930-8025-1-git-send-email-eduardo.valentin@ti.com>
On Mon, 2012-11-12 at 11:58 -0400, Eduardo Valentin wrote:
> There are predefined cpu_masks that are const data structures.
> This patch changes the cpu cooling register function so that
> those const cpu_masks can be used, without compilation warnings.
>
> include/linux/cpumask.h
>
> * The following particular system cpumasks and operations manage
> * possible, present, active and online cpus.
> *
> * cpu_possible_mask- has bit 'cpu' set iff cpu is populatable
> * cpu_present_mask - has bit 'cpu' set iff cpu is populated
> * cpu_online_mask - has bit 'cpu' set iff cpu available to scheduler
> * cpu_active_mask - has bit 'cpu' set iff cpu available to migration
> *
>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
applied.
thanks,
rui
> ---
> drivers/thermal/cpu_cooling.c | 2 +-
> include/linux/cpu_cooling.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index cc1c930..e5e7153 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -345,7 +345,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = {
> * @clip_cpus: cpumask of cpus where the frequency constraints will happen.
> */
> struct thermal_cooling_device *cpufreq_cooling_register(
> - struct cpumask *clip_cpus)
> + const struct cpumask *clip_cpus)
> {
> struct thermal_cooling_device *cool_dev;
> struct cpufreq_cooling_device *cpufreq_dev = NULL;
> diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
> index 8515301..b30cc79c 100644
> --- a/include/linux/cpu_cooling.h
> +++ b/include/linux/cpu_cooling.h
> @@ -35,7 +35,7 @@
> * @clip_cpus: cpumask of cpus where the frequency constraints will happen
> */
> struct thermal_cooling_device *cpufreq_cooling_register(
> - struct cpumask *clip_cpus);
> + const struct cpumask *clip_cpus);
>
> /**
> * cpufreq_cooling_unregister - function to remove cpufreq cooling device.
> @@ -44,7 +44,7 @@ struct thermal_cooling_device *cpufreq_cooling_register(
> void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev);
> #else /* !CONFIG_CPU_THERMAL */
> static inline struct thermal_cooling_device *cpufreq_cooling_register(
> - struct cpumask *clip_cpus)
> + const struct cpumask *clip_cpus)
> {
> return NULL;
> }
^ permalink raw reply
* Re: [PATCH v4] Thermal: exynos: Add sysfs node supporting exynos's emulation mode.
From: jonghwa3.lee @ 2012-11-21 2:19 UTC (permalink / raw)
To: Zhang Rui
Cc: Amit Kachhap, Jonghwa Lee, linux-pm, linux-kernel, Len Brown,
Durgadoss R, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park
In-Reply-To: <1353463228.2153.3.camel@rzhang1-mobl4>
On 2012년 11월 21일 11:00, Zhang Rui wrote:
> On Thu, 2012-11-08 at 14:54 +0530, Amit Kachhap wrote:
>> Hi Jonghwa Lee,
>>
>> I tested this patch and it looks good. I have some minor comments below,
>>
>> Reviewed-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>>
> Hi, Lee,
>
> I suppose there should be an updated version being sent out soon, right?
>
> Thanks,
> rui
Hi, Rui,
Yes, there is. I'll re-post it as soon.
Thanks.
>> Thanks,
>> Amit Daniel
>> On 2 November 2012 07:54, Jonghwa Lee <jonghwa3.lee@samsung.com> wrote:
>>> This patch supports exynos's emulation mode with newly created sysfs node.
>>> Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal
>>> management unit. Thermal emulation mode supports software debug for TMU's
>>> operation. User can set temperature manually with software code and TMU
>>> will read current temperature from user value not from sensor's value.
>>> This patch includes also documentary placed under Documentation/thermal/.
>>>
>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>> ---
>>> v4
>>> - Fix Typo.
>>> - Remove unnecessary codes.
>>> - Add comments about feature of exynos emulation operation to the document.
>>>
>>> v3
>>> - Remove unnecessay variables.
>>> - Do some code clean in exynos_tmu_emulation_store().
>>> - Make wrapping function of sysfs node creation function to use
>>> #ifdefs in minimum.
>>>
>>> v2
>>> exynos_thermal.c
>>> - Fix build error occured by wrong emulation control register name.
>>> - Remove exynos5410 dependent codes.
>>> exynos_thermal_emulation
>>> - Align indentation.
>>>
>>> Documentation/thermal/exynos_thermal_emulation | 56 +++++++++++++++
>>> drivers/thermal/Kconfig | 9 +++
>>> drivers/thermal/exynos_thermal.c | 91 ++++++++++++++++++++++++
>>> 3 files changed, 156 insertions(+), 0 deletions(-)
>>> create mode 100644 Documentation/thermal/exynos_thermal_emulation
>>>
>>> diff --git a/Documentation/thermal/exynos_thermal_emulation b/Documentation/thermal/exynos_thermal_emulation
>>> new file mode 100644
>>> index 0000000..a6ea06f
>>> --- /dev/null
>>> +++ b/Documentation/thermal/exynos_thermal_emulation
>>> @@ -0,0 +1,56 @@
>>> +EXYNOS EMULATION MODE
>>> +========================
>>> +
>>> +Copyright (C) 2012 Samsung Electronics
>>> +
>>> +Written by Jonghwa Lee <jonghwa3.lee@samsung.com>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal management unit.
>>> +Thermal emulation mode supports software debug for TMU's operation. User can set temperature
>>> +manually with software code and TMU will read current temperature from user value not from
>>> +sensor's value.
>>> +
>>> +Enabling CONFIG_EXYNOS_THERMAL_EMUL option will make this support in available.
>>> +When it's enabled, sysfs node will be created under
>>> +/sys/bus/platform/devices/'exynos device name'/ with name of 'emulation'.
>>> +
>>> +The sysfs node, 'emulation', will contain value 0 for the initial state. When you input any
>>> +temperature you want to update to sysfs node, it automatically enable emulation mode and
>>> +current temperature will be changed into it.
>>> +(Exynos also supports user changable delay time which would be used to delay of
>>> + changing temperature. However, this node only uses same delay of real sensing time, 938us.)
>>> +
>>> +Exynos emulation mode requires synchronous of value changing and enabling. It means when you
>>> +want to update the any value of delay or next temperature, then you have to enable emulation
>>> +mode at the same time. (Or you have to keep the mode enabling.) If you don't, it fails to
>>> +change the value to updated one and just use last succeessful value repeatedly. That's why
>>> +this node gives users the right to change termerpature only. Just one interface makes it more
>>> +simply to use.
>>> +
>>> +Disabling emulation mode only requires writing value 0 to sysfs node.
>>> +
>>> +
>>> +TEMP 120 |
>>> + |
>>> + 100 |
>>> + |
>>> + 80 |
>>> + | +-----------
>>> + 60 | | |
>>> + | +-------------| |
>>> + 40 | | | |
>>> + | | | |
>>> + 20 | | | +----------
>>> + | | | | |
>>> + 0 |______________|_____________|__________|__________|_________
>>> + A A A A TIME
>>> + |<----->| |<----->| |<----->| |
>>> + | 938us | | | | | |
>>> +emulation : 0 50 | 70 | 20 | 0
>>> +current temp : sensor 50 70 20 sensor
>>> +
>>> +
>>> +
>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>> index e1cb6bd..c02a66c 100644
>>> --- a/drivers/thermal/Kconfig
>>> +++ b/drivers/thermal/Kconfig
>>> @@ -55,3 +55,12 @@ config EXYNOS_THERMAL
>>> help
>>> If you say yes here you get support for TMU (Thermal Managment
>>> Unit) on SAMSUNG EXYNOS series of SoC.
>>> +
>>> +config EXYNOS_THERMAL_EMUL
>>> + bool "EXYNOS TMU emulation mode support"
>>> + depends on !CPU_EXYNOS4210 && EXYNOS_THERMAL
>> Instead of using CPU_EXYNOS4210 here it is better to use data->soc ==
>> SOC_ARCH_EXYNOS4210 inside the emulation show/store functions.
>>> + help
>>> + Exynos 4412 and 4414 and 5 series has emulation mode on TMU.
>>> + Enable this option will be make sysfs node in exynos thermal platform
>>> + device directory to support emulation mode. With emulation mode sysfs
>>> + node, you can manually input temperature to TMU for simulation purpose.
>>> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
>>> index fd03e85..eebd4e5 100644
>>> --- a/drivers/thermal/exynos_thermal.c
>>> +++ b/drivers/thermal/exynos_thermal.c
>>> @@ -99,6 +99,14 @@
>>> #define IDLE_INTERVAL 10000
>>> #define MCELSIUS 1000
>>>
>>> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
>>> +#define EXYNOS_EMUL_TIME 0x57F0
>>> +#define EXYNOS_EMUL_TIME_SHIFT 16
>>> +#define EXYNOS_EMUL_DATA_SHIFT 8
>>> +#define EXYNOS_EMUL_DATA_MASK 0xFF
>>> +#define EXYNOS_EMUL_ENABLE 0x1
>>> +#endif /* CONFIG_EXYNOS_THERMAL_EMUL */
>>> +
>>> /* CPU Zone information */
>>> #define PANIC_ZONE 4
>>> #define WARN_ZONE 3
>>> @@ -832,6 +840,82 @@ static inline struct exynos_tmu_platform_data *exynos_get_driver_data(
>>> return (struct exynos_tmu_platform_data *)
>>> platform_get_device_id(pdev)->driver_data;
>>> }
>>> +
>>> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
>>> +static ssize_t exynos_tmu_emulation_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct platform_device *pdev = container_of(dev,
>>> + struct platform_device, dev);
>>> + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>> + unsigned int reg;
>>> + u8 temp_code;
>>> + int temp = 0;
>>> +
>>> + mutex_lock(&data->lock);
>>> + clk_enable(data->clk);
>>> + reg = readl(data->base + EXYNOS_EMUL_CON);
>>> + clk_disable(data->clk);
>>> + mutex_unlock(&data->lock);
>>> +
>>> + if (reg & EXYNOS_EMUL_ENABLE) {
>>> + reg >>= EXYNOS_EMUL_DATA_SHIFT;
>>> + temp_code = reg & EXYNOS_EMUL_DATA_MASK;
>>> + temp = code_to_temp(data, temp_code);
>>> + }
>>> +
>>> + return sprintf(buf, "%d\n", temp);
>> Currently in /sys/devices/virtual/thermal/thermal_zone0/ all
>> temperatures are shown in millicelsius so it is better show this in
>> millicelsius also.
>>> +}
>>> +
>>> +static ssize_t exynos_tmu_emulation_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct platform_device *pdev = container_of(dev,
>>> + struct platform_device, dev);
>>> + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>> + unsigned int reg;
>>> + int temp;
>>> +
>>> + if (!sscanf(buf, "%d\n", &temp) || temp < 0)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&data->lock);
>>> + clk_enable(data->clk);
>>> +
>>> + reg = readl(data->base + EXYNOS_EMUL_CON);
>>> +
>>> + if (temp)
>>> + reg = (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT) |
>>> + (temp_to_code(data, temp) << EXYNOS_EMUL_DATA_SHIFT) |
>> Same as above.
>>> + EXYNOS_EMUL_ENABLE;
>>> + else
>>> + reg &= ~EXYNOS_EMUL_ENABLE;
>>> +
>>> + writel(reg, data->base + EXYNOS_EMUL_CON);
>>> +
>>> + clk_disable(data->clk);
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return count;
>>> +}
>>> +
>>> +static DEVICE_ATTR(emulation, 0644, exynos_tmu_emulation_show,
>>> + exynos_tmu_emulation_store);
>>> +static int create_emulation_sysfs(struct device *dev)
>>> +{
>>> + return device_create_file(dev, &dev_attr_emulation);
>>> +}
>>> +static void remove_emulation_sysfs(struct device *dev)
>>> +{
>>> + device_remove_file(dev, &dev_attr_emulation);
>>> +}
>>> +#else
>>> +static inline int create_emulation_sysfs(struct device *dev) {return 0;}
>>> +static inline void remove_emulation_sysfs(struct device *dev){}
>>> +#endif
>>> +
>>> static int __devinit exynos_tmu_probe(struct platform_device *pdev)
>>> {
>>> struct exynos_tmu_data *data;
>>> @@ -930,6 +1014,11 @@ static int __devinit exynos_tmu_probe(struct platform_device *pdev)
>>> dev_err(&pdev->dev, "Failed to register thermal interface\n");
>>> goto err_clk;
>>> }
>>> +
>>> + ret = create_emulation_sysfs(&pdev->dev);
>>> + if (ret)
>>> + dev_err(&pdev->dev, "Failed to create emulation mode sysfs node\n");
>>> +
>>> return 0;
>>> err_clk:
>>> platform_set_drvdata(pdev, NULL);
>>> @@ -941,6 +1030,8 @@ static int __devexit exynos_tmu_remove(struct platform_device *pdev)
>>> {
>>> struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>>
>>> + remove_emulation_sysfs(&pdev->dev);
>>> +
>>> exynos_tmu_control(pdev, false);
>>>
>>> exynos_unregister_thermal();
>>> --
>>> 1.7.4.1
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v4] Thermal: exynos: Add sysfs node supporting exynos's emulation mode.
From: Zhang Rui @ 2012-11-21 2:00 UTC (permalink / raw)
To: Amit Kachhap
Cc: Jonghwa Lee, linux-pm, linux-kernel, Len Brown, Durgadoss R,
Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park
In-Reply-To: <CAK44p22c_KjyGdKqrXmXNgzTPws_rR8mr-SWqJVzVgEaTbZwYg@mail.gmail.com>
On Thu, 2012-11-08 at 14:54 +0530, Amit Kachhap wrote:
> Hi Jonghwa Lee,
>
> I tested this patch and it looks good. I have some minor comments below,
>
> Reviewed-by: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>
Hi, Lee,
I suppose there should be an updated version being sent out soon, right?
Thanks,
rui
> Thanks,
> Amit Daniel
> On 2 November 2012 07:54, Jonghwa Lee <jonghwa3.lee@samsung.com> wrote:
> > This patch supports exynos's emulation mode with newly created sysfs node.
> > Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal
> > management unit. Thermal emulation mode supports software debug for TMU's
> > operation. User can set temperature manually with software code and TMU
> > will read current temperature from user value not from sensor's value.
> > This patch includes also documentary placed under Documentation/thermal/.
> >
> > Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> > ---
> > v4
> > - Fix Typo.
> > - Remove unnecessary codes.
> > - Add comments about feature of exynos emulation operation to the document.
> >
> > v3
> > - Remove unnecessay variables.
> > - Do some code clean in exynos_tmu_emulation_store().
> > - Make wrapping function of sysfs node creation function to use
> > #ifdefs in minimum.
> >
> > v2
> > exynos_thermal.c
> > - Fix build error occured by wrong emulation control register name.
> > - Remove exynos5410 dependent codes.
> > exynos_thermal_emulation
> > - Align indentation.
> >
> > Documentation/thermal/exynos_thermal_emulation | 56 +++++++++++++++
> > drivers/thermal/Kconfig | 9 +++
> > drivers/thermal/exynos_thermal.c | 91 ++++++++++++++++++++++++
> > 3 files changed, 156 insertions(+), 0 deletions(-)
> > create mode 100644 Documentation/thermal/exynos_thermal_emulation
> >
> > diff --git a/Documentation/thermal/exynos_thermal_emulation b/Documentation/thermal/exynos_thermal_emulation
> > new file mode 100644
> > index 0000000..a6ea06f
> > --- /dev/null
> > +++ b/Documentation/thermal/exynos_thermal_emulation
> > @@ -0,0 +1,56 @@
> > +EXYNOS EMULATION MODE
> > +========================
> > +
> > +Copyright (C) 2012 Samsung Electronics
> > +
> > +Written by Jonghwa Lee <jonghwa3.lee@samsung.com>
> > +
> > +Description
> > +-----------
> > +
> > +Exynos 4x12 (4212, 4412) and 5 series provide emulation mode for thermal management unit.
> > +Thermal emulation mode supports software debug for TMU's operation. User can set temperature
> > +manually with software code and TMU will read current temperature from user value not from
> > +sensor's value.
> > +
> > +Enabling CONFIG_EXYNOS_THERMAL_EMUL option will make this support in available.
> > +When it's enabled, sysfs node will be created under
> > +/sys/bus/platform/devices/'exynos device name'/ with name of 'emulation'.
> > +
> > +The sysfs node, 'emulation', will contain value 0 for the initial state. When you input any
> > +temperature you want to update to sysfs node, it automatically enable emulation mode and
> > +current temperature will be changed into it.
> > +(Exynos also supports user changable delay time which would be used to delay of
> > + changing temperature. However, this node only uses same delay of real sensing time, 938us.)
> > +
> > +Exynos emulation mode requires synchronous of value changing and enabling. It means when you
> > +want to update the any value of delay or next temperature, then you have to enable emulation
> > +mode at the same time. (Or you have to keep the mode enabling.) If you don't, it fails to
> > +change the value to updated one and just use last succeessful value repeatedly. That's why
> > +this node gives users the right to change termerpature only. Just one interface makes it more
> > +simply to use.
> > +
> > +Disabling emulation mode only requires writing value 0 to sysfs node.
> > +
> > +
> > +TEMP 120 |
> > + |
> > + 100 |
> > + |
> > + 80 |
> > + | +-----------
> > + 60 | | |
> > + | +-------------| |
> > + 40 | | | |
> > + | | | |
> > + 20 | | | +----------
> > + | | | | |
> > + 0 |______________|_____________|__________|__________|_________
> > + A A A A TIME
> > + |<----->| |<----->| |<----->| |
> > + | 938us | | | | | |
> > +emulation : 0 50 | 70 | 20 | 0
> > +current temp : sensor 50 70 20 sensor
> > +
> > +
> > +
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index e1cb6bd..c02a66c 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -55,3 +55,12 @@ config EXYNOS_THERMAL
> > help
> > If you say yes here you get support for TMU (Thermal Managment
> > Unit) on SAMSUNG EXYNOS series of SoC.
> > +
> > +config EXYNOS_THERMAL_EMUL
> > + bool "EXYNOS TMU emulation mode support"
> > + depends on !CPU_EXYNOS4210 && EXYNOS_THERMAL
> Instead of using CPU_EXYNOS4210 here it is better to use data->soc ==
> SOC_ARCH_EXYNOS4210 inside the emulation show/store functions.
> > + help
> > + Exynos 4412 and 4414 and 5 series has emulation mode on TMU.
> > + Enable this option will be make sysfs node in exynos thermal platform
> > + device directory to support emulation mode. With emulation mode sysfs
> > + node, you can manually input temperature to TMU for simulation purpose.
> > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
> > index fd03e85..eebd4e5 100644
> > --- a/drivers/thermal/exynos_thermal.c
> > +++ b/drivers/thermal/exynos_thermal.c
> > @@ -99,6 +99,14 @@
> > #define IDLE_INTERVAL 10000
> > #define MCELSIUS 1000
> >
> > +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
> > +#define EXYNOS_EMUL_TIME 0x57F0
> > +#define EXYNOS_EMUL_TIME_SHIFT 16
> > +#define EXYNOS_EMUL_DATA_SHIFT 8
> > +#define EXYNOS_EMUL_DATA_MASK 0xFF
> > +#define EXYNOS_EMUL_ENABLE 0x1
> > +#endif /* CONFIG_EXYNOS_THERMAL_EMUL */
> > +
> > /* CPU Zone information */
> > #define PANIC_ZONE 4
> > #define WARN_ZONE 3
> > @@ -832,6 +840,82 @@ static inline struct exynos_tmu_platform_data *exynos_get_driver_data(
> > return (struct exynos_tmu_platform_data *)
> > platform_get_device_id(pdev)->driver_data;
> > }
> > +
> > +#ifdef CONFIG_EXYNOS_THERMAL_EMUL
> > +static ssize_t exynos_tmu_emulation_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct platform_device *pdev = container_of(dev,
> > + struct platform_device, dev);
> > + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > + unsigned int reg;
> > + u8 temp_code;
> > + int temp = 0;
> > +
> > + mutex_lock(&data->lock);
> > + clk_enable(data->clk);
> > + reg = readl(data->base + EXYNOS_EMUL_CON);
> > + clk_disable(data->clk);
> > + mutex_unlock(&data->lock);
> > +
> > + if (reg & EXYNOS_EMUL_ENABLE) {
> > + reg >>= EXYNOS_EMUL_DATA_SHIFT;
> > + temp_code = reg & EXYNOS_EMUL_DATA_MASK;
> > + temp = code_to_temp(data, temp_code);
> > + }
> > +
> > + return sprintf(buf, "%d\n", temp);
> Currently in /sys/devices/virtual/thermal/thermal_zone0/ all
> temperatures are shown in millicelsius so it is better show this in
> millicelsius also.
> > +}
> > +
> > +static ssize_t exynos_tmu_emulation_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct platform_device *pdev = container_of(dev,
> > + struct platform_device, dev);
> > + struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> > + unsigned int reg;
> > + int temp;
> > +
> > + if (!sscanf(buf, "%d\n", &temp) || temp < 0)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->lock);
> > + clk_enable(data->clk);
> > +
> > + reg = readl(data->base + EXYNOS_EMUL_CON);
> > +
> > + if (temp)
> > + reg = (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT) |
> > + (temp_to_code(data, temp) << EXYNOS_EMUL_DATA_SHIFT) |
> Same as above.
> > + EXYNOS_EMUL_ENABLE;
> > + else
> > + reg &= ~EXYNOS_EMUL_ENABLE;
> > +
> > + writel(reg, data->base + EXYNOS_EMUL_CON);
> > +
> > + clk_disable(data->clk);
> > + mutex_unlock(&data->lock);
> > +
> > + return count;
> > +}
> > +
> > +static DEVICE_ATTR(emulation, 0644, exynos_tmu_emulation_show,
> > + exynos_tmu_emulation_store);
> > +static int create_emulation_sysfs(struct device *dev)
> > +{
> > + return device_create_file(dev, &dev_attr_emulation);
> > +}
> > +static void remove_emulation_sysfs(struct device *dev)
> > +{
> > + device_remove_file(dev, &dev_attr_emulation);
> > +}
> > +#else
> > +static inline int create_emulation_sysfs(struct device *dev) {return 0;}
> > +static inline void remove_emulation_sysfs(struct device *dev){}
> > +#endif
> > +
> > static int __devinit exynos_tmu_probe(struct platform_device *pdev)
> > {
> > struct exynos_tmu_data *data;
> > @@ -930,6 +1014,11 @@ static int __devinit exynos_tmu_probe(struct platform_device *pdev)
> > dev_err(&pdev->dev, "Failed to register thermal interface\n");
> > goto err_clk;
> > }
> > +
> > + ret = create_emulation_sysfs(&pdev->dev);
> > + if (ret)
> > + dev_err(&pdev->dev, "Failed to create emulation mode sysfs node\n");
> > +
> > return 0;
> > err_clk:
> > platform_set_drvdata(pdev, NULL);
> > @@ -941,6 +1030,8 @@ static int __devexit exynos_tmu_remove(struct platform_device *pdev)
> > {
> > struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> >
> > + remove_emulation_sysfs(&pdev->dev);
> > +
> > exynos_tmu_control(pdev, false);
> >
> > exynos_unregister_thermal();
> > --
> > 1.7.4.1
> >
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-11-21 1:56 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Anton Vorontsov, Stephen Warren, Thierry Reding, Mark Zhang,
Grant Likely, Rob Herring, Mark Brown, David Woodhouse,
Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot
In-Reply-To: <50AB9832.90709-l0cyMroinI0@public.gmane.org>
Hi Tomi,
On Tuesday 20 November 2012 22:48:18 Tomi Valkeinen wrote:
> I guess there's a reason, but the above looks a bit inconsistent. For
> gpio you define the gpio resource inside the step. For power and pwm the
> resource is defined before the steps. Why wouldn't "pwm = <&pwm 2
> 5000000>;" work in step2?
That's mostly a framework issue. Most frameworks do not export a function that
allow to dereference a phandle - they expect resources to be declared right
under the device node and accessed by name through foo_get(device, name). So
using phandles in power sequences would require to export these additional
functions and also opens the door to some inconsistencies - for instance, your
PWM phandle could be referenced a second time in the sequence with a different
period - how do you know that these are actually referring the same PWM
device?
> > +When a power sequence is run, its steps is executed one after the other
> > until +one step fails or the end of the sequence is reached.
>
> The document doesn't give any hint of what the driver should do if
> running the power sequence fails. Run the "opposite" power sequence?
> Will that work for all resources? I'm mainly thinking of a case where
> each enable of the resource should be matched by a disable, i.e. you
> can't call disable if no enable was called.
We discussed that issue already (around v5 I think) and the conclusion was
that it should be up to the driver. When we simply enable/disable resources it
is easy to revert, but in the future non-boolean properties will likely be
introduced, and these cannot easily be reverted. Moreover some drivers might
have more complex recovery needs. This deserves more discussion I think, as
I'd like to have some "generic" recovery mechanism that covers most of the
cases.
Alex.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Mark Brown @ 2012-11-21 1:31 UTC (permalink / raw)
To: Grant Likely
Cc: Alexandre Courbot, Anton Vorontsov, Stephen Warren,
Thierry Reding, Mark Zhang, Rob Herring, David Woodhouse,
Arnd Bergmann, linux-tegra, linux-arm-kernel, linux-kernel,
linux-fbdev, devicetree-discuss, linux-pm, Alexandre Courbot
In-Reply-To: <20121120215429.B621F3E1821@localhost>
[-- Attachment #1: Type: text/plain, Size: 2354 bytes --]
On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote:
> On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> > With the advent of the device tree and of ARM kernels that are not
> > board-tied, we cannot rely on these board-specific hooks anymore but
> This isn't strictly true. It is still perfectly fine to have board
> specific code when necessary. However, there is strong encouragement to
> enable that code in device drivers as much as possible and new board
> files need to have very strong justification.
This isn't the message that's gone over, and even for device drivers
everyone seems to be taking the whole device tree thing as a move to
pull all data out of the kernel. In some cases there are some real
practical advantages to doing this but a lot of the people making these
changes seem to view having things in DT as a goal in itself.
> I'm thinking about the scripts as trivial-to-parse ascii strings that
> have a very simple set of commands. The commands use resources already
> defined in the node. ie. 'g0' refers to the first entry in the gpios
> property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> no means take this as the format to use, it is just an example off the
> top of my head, but it is already way easier to work with than putting
> each command into a node.
It does appear to have some legibility challenges, especially with using
the indexes into the arrays of resources. On the other hand the arrays
should be fairly small.
> > +Platform Data Format
> > +--------------------
> > +All relevant data structures for declaring power sequences are located in
> > +include/linux/power_seq.h.
> Hmm... If sequences are switched to a string instead, then platform_data
> should also use it. The power sequence data structures can be created at
> runtime by parsing the string.
Seems like a step backwards to me - why not let people store the more
parsed version of the data? It's going to be less convenient for users
on non-DT systems.
As I said in my earlier reviews I think this is a useful thing to have
as a utility library for drivers independantly of the DT bindings, it
would allow drivers to become more data driven. Perhaps we can rework
the series so that the DT bindings are added as a final patch? All the
rest of the code seems OK.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ 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