* Re: [PATCH 3/5] PM / QoS: Add function dev_pm_qos_read_value()
From: Rafael J. Wysocki @ 2011-09-05 15:30 UTC (permalink / raw)
To: Jean Pihet
Cc: Linux PM mailing list, LKML, Linux-sh list, Magnus Damm,
Kevin Hilman
In-Reply-To: <CAORVsuXtgd6hTvpkoHobSGKrXgMB1L8dSVbkkt071useNRdngg@mail.gmail.com>
On Monday, September 05, 2011, Jean Pihet wrote:
> Hi,
>
> On Sat, Sep 3, 2011 at 10:02 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Hi,
> >
> > On Saturday, September 03, 2011, Rafael J. Wysocki wrote:
> >> On Friday, September 02, 2011, Jean Pihet wrote:
> >> > On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > ...
> >> > >> >
> >> > >> > - mutex_lock(&dev_pm_qos_mtx);
> >> > >> > req->dev = dev;
> >> > >> >
> >> > >> > - /* Return if the device has been removed */
> >> > >> > - if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
> >> > >> > - ret = -ENODEV;
> >> > >> > - goto out;
> >> > >> > - }
> >> > >> > + device_pm_lock();
> >> > >> > + mutex_lock(&dev_pm_qos_mtx);
> >> > >> >
> >> > >> > - /*
> >> > >> > - * Allocate the constraints data on the first call to add_request,
> >> > >> > - * i.e. only if the data is not already allocated and if the device has
> >> > >> > - * not been removed
> >> > >> > - */
> >> > >> > - if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
> >> > >> > - ret = dev_pm_qos_constraints_allocate(dev);
> >> > >> > + if (dev->power.constraints) {
> >> > >> > + device_pm_unlock();
> >> > >> > + } else {
> >> > >> > + if (list_empty(&dev->power.entry)) {
> >> > >> > + /* The device has been removed from the system. */
> >> > >> > + device_pm_unlock();
> >> > >> > + goto out;
> >> > >> 0 is silently returned in case the device has been removed. Is that
> >> > >> the intention?
> >> > >
> >> > > Pretty much it is. Is that a problem?
> >> > I think the caller needs to know if the constraint has been applied
> >> > correctly or not.
> >>
> >> However, the removal of the device is a special case. What would the caller
> >> be supposed to do when it learned that the device had been removed while it had
> >> been trying to add a QoS constraing for it? Not much I guess.
> >
> > Anyway, since returning an error code in that case won't hurt either,
> > below is a v2 of the patch doing that and resetting the dev field in the
> > req structure.
> Ok thanks for the update. The comments are inlined below.
>
...
> >
> > - if (req->dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
> > + if (req->dev->power.constraints) {
> > if (new_value != req->node.prio)
> > ret = apply_constraint(req, PM_QOS_UPDATE_REQ,
> > new_value);
> > } else {
> > /* Return if the device has been removed */
> > - ret = -ENODEV;
> > + ret = -EINVAL;
> The retcode should be -ENODEV.
Well, both -EINVAL and -ENODEV seem somewhat suitable here and I don't
have a strong preference, so I very well may leave the latter as it.
> Is the kerneldoc still OK?
It will be if the error code isn't changed.
Updated patch follows.
Thanks,
Rafael
---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / QoS: Add function dev_pm_qos_read_value() (v2)
To read the current PM QoS value for a given device we need to
make sure that the device's power.constraints object won't be
removed while we're doing that. For this reason, put the
operation under dev->power.lock and acquire the lock
around the initialization and removal of power.constraints.
Moreover, since we're using the value of power.constraints to
determine whether or not the object is present, the
power.constraints_state field isn't necessary any more and may be
removed. However, dev_pm_qos_add_request() needs to check if the
device is being removed from the system before allocating a new
PM QoS constraints object for it, so it has to use device_pm_lock()
and the device PM QoS initialization and destruction should be done
under device_pm_lock() as well.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
drivers/base/power/main.c | 4 -
drivers/base/power/qos.c | 168 ++++++++++++++++++++++++++--------------------
include/linux/pm.h | 8 --
include/linux/pm_qos.h | 3
4 files changed, 103 insertions(+), 80 deletions(-)
Index: linux/drivers/base/power/qos.c
=================================--- linux.orig/drivers/base/power/qos.c
+++ linux/drivers/base/power/qos.c
@@ -30,15 +30,6 @@
* . To minimize the data usage by the per-device constraints, the data struct
* is only allocated at the first call to dev_pm_qos_add_request.
* . The data is later free'd when the device is removed from the system.
- * . The constraints_state variable from dev_pm_info tracks the data struct
- * allocation state:
- * DEV_PM_QOS_NO_DEVICE: No device present or device removed, no data
- * allocated,
- * DEV_PM_QOS_DEVICE_PRESENT: Device present, data not allocated and will be
- * allocated at the first call to dev_pm_qos_add_request,
- * DEV_PM_QOS_ALLOCATED: Device present, data allocated. The per-device
- * PM QoS constraints framework is operational and constraints can be
- * added, updated or removed using the dev_pm_qos_* API.
* . A global mutex protects the constraints users from the data being
* allocated and free'd.
*/
@@ -51,8 +42,30 @@
static DEFINE_MUTEX(dev_pm_qos_mtx);
+
static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
+/**
+ * dev_pm_qos_read_value - Get PM QoS constraint for a given device.
+ * @dev: Device to get the PM QoS constraint value for.
+ */
+s32 dev_pm_qos_read_value(struct device *dev)
+{
+ struct pm_qos_constraints *c;
+ unsigned long flags;
+ s32 ret = 0;
+
+ spin_lock_irqsave(&dev->power.lock, flags);
+
+ c = dev->power.constraints;
+ if (c)
+ ret = pm_qos_read_value(c);
+
+ spin_unlock_irqrestore(&dev->power.lock, flags);
+
+ return ret;
+}
+
/*
* apply_constraint
* @req: constraint request to apply
@@ -105,27 +118,37 @@ static int dev_pm_qos_constraints_alloca
}
BLOCKING_INIT_NOTIFIER_HEAD(n);
+ plist_head_init(&c->list);
+ c->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+ c->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
+ c->type = PM_QOS_MIN;
+ c->notifiers = n;
+
+ spin_lock_irq(&dev->power.lock);
dev->power.constraints = c;
- plist_head_init(&dev->power.constraints->list);
- dev->power.constraints->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
- dev->power.constraints->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
- dev->power.constraints->type = PM_QOS_MIN;
- dev->power.constraints->notifiers = n;
- dev->power.constraints_state = DEV_PM_QOS_ALLOCATED;
+ spin_unlock_irq(&dev->power.lock);
return 0;
}
+static void __dev_pm_qos_constraints_init(struct device *dev)
+{
+ spin_lock_irq(&dev->power.lock);
+ dev->power.constraints = NULL;
+ spin_unlock_irq(&dev->power.lock);
+}
+
/**
- * dev_pm_qos_constraints_init
+ * dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
* @dev: target device
*
- * Called from the device PM subsystem at device insertion
+ * Called from the device PM subsystem at device insertion under
+ * device_pm_lock().
*/
void dev_pm_qos_constraints_init(struct device *dev)
{
mutex_lock(&dev_pm_qos_mtx);
- dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
+ dev->power.constraints = NULL;
mutex_unlock(&dev_pm_qos_mtx);
}
@@ -133,34 +156,35 @@ void dev_pm_qos_constraints_init(struct
* dev_pm_qos_constraints_destroy
* @dev: target device
*
- * Called from the device PM subsystem at device removal
+ * Called from the device PM subsystem at device removal under device_pm_lock().
*/
void dev_pm_qos_constraints_destroy(struct device *dev)
{
struct dev_pm_qos_request *req, *tmp;
+ struct pm_qos_constraints *c;
mutex_lock(&dev_pm_qos_mtx);
- if (dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
- /* Flush the constraints list for the device */
- plist_for_each_entry_safe(req, tmp,
- &dev->power.constraints->list,
- node) {
- /*
- * Update constraints list and call the notification
- * callbacks if needed
- */
- apply_constraint(req, PM_QOS_REMOVE_REQ,
- PM_QOS_DEFAULT_VALUE);
- memset(req, 0, sizeof(*req));
- }
+ c = dev->power.constraints;
+ if (!c)
+ goto out;
- kfree(dev->power.constraints->notifiers);
- kfree(dev->power.constraints);
- dev->power.constraints = NULL;
+ /* Flush the constraints list for the device */
+ plist_for_each_entry_safe(req, tmp, &c->list, node) {
+ /*
+ * Update constraints list and call the notification
+ * callbacks if needed
+ */
+ apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+ memset(req, 0, sizeof(*req));
}
- dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
+ __dev_pm_qos_constraints_init(dev);
+
+ kfree(c->notifiers);
+ kfree(c);
+
+ out:
mutex_unlock(&dev_pm_qos_mtx);
}
@@ -178,8 +202,9 @@ void dev_pm_qos_constraints_destroy(stru
*
* Returns 1 if the aggregated constraint value has changed,
* 0 if the aggregated constraint value has not changed,
- * -EINVAL in case of wrong parameters, -ENODEV if the device has been
- * removed from the system
+ * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
+ * to allocate for data structures, -ENODEV if the device has just been removed
+ * from the system.
*/
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
s32 value)
@@ -195,28 +220,37 @@ int dev_pm_qos_add_request(struct device
return -EINVAL;
}
- mutex_lock(&dev_pm_qos_mtx);
req->dev = dev;
- /* Return if the device has been removed */
- if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
- ret = -ENODEV;
- goto out;
- }
+ device_pm_lock();
+ mutex_lock(&dev_pm_qos_mtx);
- /*
- * Allocate the constraints data on the first call to add_request,
- * i.e. only if the data is not already allocated and if the device has
- * not been removed
- */
- if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
- ret = dev_pm_qos_constraints_allocate(dev);
+ if (dev->power.constraints) {
+ device_pm_unlock();
+ } else {
+ if (list_empty(&dev->power.entry)) {
+ /* The device has been removed from the system. */
+ device_pm_unlock();
+ req->dev = NULL;
+ ret = -ENODEV;
+ goto out;
+ } else {
+ device_pm_unlock();
+ /*
+ * Allocate the constraints data on the first call to
+ * add_request, i.e. only if the data is not already
+ * allocated and if the device has not been removed.
+ */
+ ret = dev_pm_qos_constraints_allocate(dev);
+ }
+ }
if (!ret)
ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
-out:
+ out:
mutex_unlock(&dev_pm_qos_mtx);
+
return ret;
}
EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
@@ -252,7 +286,7 @@ int dev_pm_qos_update_request(struct dev
mutex_lock(&dev_pm_qos_mtx);
- if (req->dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
+ if (req->dev->power.constraints) {
if (new_value != req->node.prio)
ret = apply_constraint(req, PM_QOS_UPDATE_REQ,
new_value);
@@ -293,7 +327,7 @@ int dev_pm_qos_remove_request(struct dev
mutex_lock(&dev_pm_qos_mtx);
- if (req->dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
+ if (req->dev->power.constraints) {
ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
PM_QOS_DEFAULT_VALUE);
memset(req, 0, sizeof(*req));
@@ -323,15 +357,12 @@ int dev_pm_qos_add_notifier(struct devic
mutex_lock(&dev_pm_qos_mtx);
- /* Silently return if the device has been removed */
- if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
- goto out;
-
- retval = blocking_notifier_chain_register(
- dev->power.constraints->notifiers,
- notifier);
+ /* Silently return if the constraints object is not present. */
+ if (dev->power.constraints)
+ retval = blocking_notifier_chain_register(
+ dev->power.constraints->notifiers,
+ notifier);
-out:
mutex_unlock(&dev_pm_qos_mtx);
return retval;
}
@@ -354,15 +385,12 @@ int dev_pm_qos_remove_notifier(struct de
mutex_lock(&dev_pm_qos_mtx);
- /* Silently return if the device has been removed */
- if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
- goto out;
-
- retval = blocking_notifier_chain_unregister(
- dev->power.constraints->notifiers,
- notifier);
+ /* Silently return if the constraints object is not present. */
+ if (dev->power.constraints)
+ retval = blocking_notifier_chain_unregister(
+ dev->power.constraints->notifiers,
+ notifier);
-out:
mutex_unlock(&dev_pm_qos_mtx);
return retval;
}
Index: linux/include/linux/pm_qos.h
=================================--- linux.orig/include/linux/pm_qos.h
+++ linux/include/linux/pm_qos.h
@@ -77,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_cl
int pm_qos_request_active(struct pm_qos_request *req);
s32 pm_qos_read_value(struct pm_qos_constraints *c);
+s32 dev_pm_qos_read_value(struct device *dev);
int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
s32 value);
int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
@@ -117,6 +118,8 @@ static inline int pm_qos_request_active(
static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
{ return 0; }
+static inline s32 dev_pm_qos_read_value(struct device *dev)
+ { return 0; }
static inline int dev_pm_qos_add_request(struct device *dev,
struct dev_pm_qos_request *req,
s32 value)
Index: linux/drivers/base/power/main.c
=================================--- linux.orig/drivers/base/power/main.c
+++ linux/drivers/base/power/main.c
@@ -98,8 +98,8 @@ void device_pm_add(struct device *dev)
dev_warn(dev, "parent %s should not be sleeping\n",
dev_name(dev->parent));
list_add_tail(&dev->power.entry, &dpm_list);
- mutex_unlock(&dpm_list_mtx);
dev_pm_qos_constraints_init(dev);
+ mutex_unlock(&dpm_list_mtx);
}
/**
@@ -110,9 +110,9 @@ void device_pm_remove(struct device *dev
{
pr_debug("PM: Removing info for %s:%s\n",
dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
- dev_pm_qos_constraints_destroy(dev);
complete_all(&dev->power.completion);
mutex_lock(&dpm_list_mtx);
+ dev_pm_qos_constraints_destroy(dev);
list_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
device_wakeup_disable(dev);
Index: linux/include/linux/pm.h
=================================--- linux.orig/include/linux/pm.h
+++ linux/include/linux/pm.h
@@ -421,13 +421,6 @@ enum rpm_request {
RPM_REQ_RESUME,
};
-/* Per-device PM QoS constraints data struct state */
-enum dev_pm_qos_state {
- DEV_PM_QOS_NO_DEVICE, /* No device present */
- DEV_PM_QOS_DEVICE_PRESENT, /* Device present, data not allocated */
- DEV_PM_QOS_ALLOCATED, /* Device present, data allocated */
-};
-
struct wakeup_source;
struct pm_domain_data {
@@ -489,7 +482,6 @@ struct dev_pm_info {
#endif
struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
struct pm_qos_constraints *constraints;
- enum dev_pm_qos_state constraints_state;
};
extern void update_pm_runtime_accounting(struct device *dev);
^ permalink raw reply
* Re: [PATCH] dma: shdma: transfer based runtime PM
From: Vinod Koul @ 2011-09-05 15:25 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: linux-kernel, linux-sh, Dan Williams, Paul Mundt
In-Reply-To: <1315227964.26251.68.camel@vkoul-udesk3>
On Mon, 2011-09-05 at 18:36 +0530, Vinod Koul wrote:
> On Mon, 2011-09-05 at 10:10 +0200, Guennadi Liakhovetski wrote:
> > Hi Vinod
> >
> > On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> Please dont top post...
> > > Sorry, I see it differently. I don't use any counters in my patch. I'm
> > > only checking for empty queue, i.e., I'm just identifying the first
> > > descriptor submission and the last completion or termination.
> > >
> > > > You juts need
> > > > to call _put/_get at right places, which IMO l;ooks lot simpler than
> > > > current approach
> > >
> > > If we didn't have to check for exact symmetry, then yes, I agree, this
> > > would be cleaner. I.e., if we indeed had well-defined entry- and
> > > exit-points, which are guaranteed to be called exact same number of times.
> > > Like, e.g., with file open() / close() etc. But since we don't have this
> > > symmetry, and instead have to add flags and iterate lists, this doesn't
> > > look natural and simple to me anymore, sorry.
> >
> > What about this one? Would you be prepared to take it as is, or you still
> > think, that a pm_runtime_get*() on each descriptor submission would be
> > better?
> I think I will go with your current approach. Let me review again and
> check it. If I get time it should be in my tree by tonight
This patch fails to apply for me, can you please rebase it to me tree
and resend, I will apply it later this week
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dma: shdma: transfer based runtime PM
From: Guennadi Liakhovetski @ 2011-09-05 15:23 UTC (permalink / raw)
To: Vinod Koul; +Cc: linux-kernel, linux-sh, Dan Williams, Paul Mundt
In-Reply-To: <1315235596.26251.201.camel@vkoul-udesk3>
On Mon, 5 Sep 2011, Vinod Koul wrote:
> On Mon, 2011-09-05 at 18:36 +0530, Vinod Koul wrote:
> > On Mon, 2011-09-05 at 10:10 +0200, Guennadi Liakhovetski wrote:
> > > Hi Vinod
> > >
> > > On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> > Please dont top post...
> > > > Sorry, I see it differently. I don't use any counters in my patch. I'm
> > > > only checking for empty queue, i.e., I'm just identifying the first
> > > > descriptor submission and the last completion or termination.
> > > >
> > > > > You juts need
> > > > > to call _put/_get at right places, which IMO l;ooks lot simpler than
> > > > > current approach
> > > >
> > > > If we didn't have to check for exact symmetry, then yes, I agree, this
> > > > would be cleaner. I.e., if we indeed had well-defined entry- and
> > > > exit-points, which are guaranteed to be called exact same number of times.
> > > > Like, e.g., with file open() / close() etc. But since we don't have this
> > > > symmetry, and instead have to add flags and iterate lists, this doesn't
> > > > look natural and simple to me anymore, sorry.
> > >
> > > What about this one? Would you be prepared to take it as is, or you still
> > > think, that a pm_runtime_get*() on each descriptor submission would be
> > > better?
> > I think I will go with your current approach. Let me review again and
> > check it. If I get time it should be in my tree by tonight
> This patch fails to apply for me, can you please rebase it to me tree
> and resend, I will apply it later this week
Sure, will do.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Guennadi Liakhovetski @ 2011-09-05 15:21 UTC (permalink / raw)
To: Vinod Koul
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <1315235335.26251.194.camel@vkoul-udesk3>
On Mon, 5 Sep 2011, Vinod Koul wrote:
> On Mon, 2011-09-05 at 17:01 +0200, Guennadi Liakhovetski wrote:
> > On Mon, 5 Sep 2011, Vinod Koul wrote:
> >
> > > On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > > >
> > > > Let me try again. DMA channels on these DMA controllers are not dedicated.
> > > > On one such SoC there can be several such DMA controllers of different
> > > > kinds. One kind is "generic" - it can do memcpy(), besides channels can be
> > > > freely configured for one of onboard peripherals: serial, mmc, etc. Some
> > > > of them can also serve external DMA-capable devices. Another kind of DMA
> > > > controllers, served by the same driver, can only be used with USB
> > > > controllers. Now, if the MMC driver requests a DMA channel, let's say, the
> > > > dmaengine core first finds the USB DMA controller. The MMC driver cannot
> > > > know this. It assigns its MMC DMA configuration to the.private pointer and
> > > > returns true. Next the DMA driver is entered, it checks the private
> > > > pointer, sees an MMC channel request, looks at the DMA controller and
> > > > sees, that it doesn't support MMC. So, .device_alloc_chan_resources()
> > > > fails. When the same is attempted with a suitable DMA controller, the
> > > > shdma driver recognises, that the controller can service MMC and uses the
> > > > data, provided the MMC driver, to configure the DMA channel for MMC.
> > > Hmmm, Can't you know in filter function if the respective channel can do
> > > the dma for you or not? Maybe export a dma function or use platform data
> > > for this (wont you soc have these caps fixed), i prefer latter.
> > > That maybe a better approach.
> >
> > How? On a system you can have 3 suitable DMA controllers and 2 unsuitable.
> > Do you want to pass a list of 3 suitable DMA controllers to each
> > peripheral driver?...
> The peripheral driver (client driver in slave-dma terminology) should
> already know which dmac it wants. (base on information in platform data
> etc) That is why the filter function is provided. Please use it properly
> Other soc have similar capabilities and they can filter properly so why
> cant you..?
Sorry, I have been thinking about these possibilities, but I really didn't
find any similar case in existing drivers. Normally either channels are
fixed - only one channel can be used for a specific peripheral, or any at
all, or there is only one suitable controller. I only see two
possibilities here, and they both look ugly to me:
(1) pass a list of suitable DMA controllers to slave-dma drivers, there in
the filter you'd have to scan that list.
(2) select only one out of several suitable DMA controllers in the
platform configuration - that needlessly reduces flexibility.
Whereas on the contrary, the DMA controller itself can perfectly look
through the list of supported peripherals on the current controller and
decide, whether the requested one is among them or not.
> PS: This might well be my last post before Tue EOD PST, traveling to
> LPC, if you are there feel free to chat with me on this.
No, unfortunately, I won#t be there. Are you coming to the KS in Prague in
October?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Vinod Koul @ 2011-09-05 15:20 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <Pine.LNX.4.64.1109051700030.1112@axis700.grange>
On Mon, 2011-09-05 at 17:01 +0200, Guennadi Liakhovetski wrote:
> On Mon, 5 Sep 2011, Vinod Koul wrote:
>
> > On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> > >
> > > Let me try again. DMA channels on these DMA controllers are not dedicated.
> > > On one such SoC there can be several such DMA controllers of different
> > > kinds. One kind is "generic" - it can do memcpy(), besides channels can be
> > > freely configured for one of onboard peripherals: serial, mmc, etc. Some
> > > of them can also serve external DMA-capable devices. Another kind of DMA
> > > controllers, served by the same driver, can only be used with USB
> > > controllers. Now, if the MMC driver requests a DMA channel, let's say, the
> > > dmaengine core first finds the USB DMA controller. The MMC driver cannot
> > > know this. It assigns its MMC DMA configuration to the.private pointer and
> > > returns true. Next the DMA driver is entered, it checks the private
> > > pointer, sees an MMC channel request, looks at the DMA controller and
> > > sees, that it doesn't support MMC. So, .device_alloc_chan_resources()
> > > fails. When the same is attempted with a suitable DMA controller, the
> > > shdma driver recognises, that the controller can service MMC and uses the
> > > data, provided the MMC driver, to configure the DMA channel for MMC.
> > Hmmm, Can't you know in filter function if the respective channel can do
> > the dma for you or not? Maybe export a dma function or use platform data
> > for this (wont you soc have these caps fixed), i prefer latter.
> > That maybe a better approach.
>
> How? On a system you can have 3 suitable DMA controllers and 2 unsuitable.
> Do you want to pass a list of 3 suitable DMA controllers to each
> peripheral driver?...
The peripheral driver (client driver in slave-dma terminology) should
already know which dmac it wants. (base on information in platform data
etc) That is why the filter function is provided. Please use it properly
Other soc have similar capabilities and they can filter properly so why
cant you..?
--
~Vinod
PS: This might well be my last post before Tue EOD PST, traveling to
LPC, if you are there feel free to chat with me on this.
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Guennadi Liakhovetski @ 2011-09-05 15:01 UTC (permalink / raw)
To: Vinod Koul
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <1315233788.26251.163.camel@vkoul-udesk3>
On Mon, 5 Sep 2011, Vinod Koul wrote:
> On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
> >
> > Let me try again. DMA channels on these DMA controllers are not dedicated.
> > On one such SoC there can be several such DMA controllers of different
> > kinds. One kind is "generic" - it can do memcpy(), besides channels can be
> > freely configured for one of onboard peripherals: serial, mmc, etc. Some
> > of them can also serve external DMA-capable devices. Another kind of DMA
> > controllers, served by the same driver, can only be used with USB
> > controllers. Now, if the MMC driver requests a DMA channel, let's say, the
> > dmaengine core first finds the USB DMA controller. The MMC driver cannot
> > know this. It assigns its MMC DMA configuration to the.private pointer and
> > returns true. Next the DMA driver is entered, it checks the private
> > pointer, sees an MMC channel request, looks at the DMA controller and
> > sees, that it doesn't support MMC. So, .device_alloc_chan_resources()
> > fails. When the same is attempted with a suitable DMA controller, the
> > shdma driver recognises, that the controller can service MMC and uses the
> > data, provided the MMC driver, to configure the DMA channel for MMC.
> Hmmm, Can't you know in filter function if the respective channel can do
> the dma for you or not? Maybe export a dma function or use platform data
> for this (wont you soc have these caps fixed), i prefer latter.
> That maybe a better approach.
How? On a system you can have 3 suitable DMA controllers and 2 unsuitable.
Do you want to pass a list of 3 suitable DMA controllers to each
peripheral driver?...
> Once you have been allocated it should normally work, additional
> filtering confuses :(
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Vinod Koul @ 2011-09-05 14:55 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <Pine.LNX.4.64.1109051532220.1112@axis700.grange>
On Mon, 2011-09-05 at 15:48 +0200, Guennadi Liakhovetski wrote:
>
> Let me try again. DMA channels on these DMA controllers are not dedicated.
> On one such SoC there can be several such DMA controllers of different
> kinds. One kind is "generic" - it can do memcpy(), besides channels can be
> freely configured for one of onboard peripherals: serial, mmc, etc. Some
> of them can also serve external DMA-capable devices. Another kind of DMA
> controllers, served by the same driver, can only be used with USB
> controllers. Now, if the MMC driver requests a DMA channel, let's say, the
> dmaengine core first finds the USB DMA controller. The MMC driver cannot
> know this. It assigns its MMC DMA configuration to the.private pointer and
> returns true. Next the DMA driver is entered, it checks the private
> pointer, sees an MMC channel request, looks at the DMA controller and
> sees, that it doesn't support MMC. So, .device_alloc_chan_resources()
> fails. When the same is attempted with a suitable DMA controller, the
> shdma driver recognises, that the controller can service MMC and uses the
> data, provided the MMC driver, to configure the DMA channel for MMC.
Hmmm, Can't you know in filter function if the respective channel can do
the dma for you or not? Maybe export a dma function or use platform data
for this (wont you soc have these caps fixed), i prefer latter.
That maybe a better approach.
Once you have been allocated it should normally work, additional
filtering confuses :(
--
~Vinod
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Guennadi Liakhovetski @ 2011-09-05 13:48 UTC (permalink / raw)
To: Vinod Koul
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <1315228408.26251.75.camel@vkoul-udesk3>
On Mon, 5 Sep 2011, Vinod Koul wrote:
> On Mon, 2011-09-05 at 10:04 +0200, Guennadi Liakhovetski wrote:
> > Hi Vinod
> >
> > On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> >
> > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > >
> > > > On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > > > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > > > >
> > > > > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > > > > >> You should not assign chan->private.
> > > > > > > > > >> Please move this to dma_slave_control API
> > > > > > > > > >
> > > > > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > > > > I don't recall seeing any updated version fixing this
> > > > > > >
> > > > > > > As a matter of fact, when you say "use dma_slave_control API," you
> > > > > > > actually mean the dma_slave_config, right? Then, how is one supposed to
> > > > > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of
> > > > > > > the filter and check the return code? The problem is, that not all DMA
> > > > > > > controllers on sh-mobile SoCs can service the same slave devices. So, if
> > > > > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > > > > Here you are assigning to chan->private your specific values, which
> > > > > > should be moved to the dma_slave_config.
> > > > > > But here you are removing the checks, and accepting the first channel
> > > > > > you got, so how do you find channel is suitable or not.
> > > > >
> > > > > That's done in the driver's .device_alloc_chan_resources() method. It
> > > > > checkx the .private pointer, tries to find a suitable channel, if it fails
> > > > > - it returns -EINVAL. See
> > > > > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > > > >
> > > > > if (param) {
> > > > > const struct sh_dmae_slave_config *cfg;
> > > > >
> > > > > cfg = sh_dmae_find_slave(sh_chan, param);
> > > > > if (!cfg) {
> > > > > ret = -EINVAL;
> > > > > goto efindslave;
> > > > > }
> > > > Now am doubly confused. Are you saying that you are using
> > > > alloc_chan_resources for doing filtering??
> > >
> > > Let's look at __dma_request_channel(). It iterates over DMA devices and
> > > calls private_candidate() for each of them, which in turn calls the filter
> > > function for each free channel on the current device. As soon as the
> > > filter returns "true" for one of channels, a private candidate is found.
> > > And in my filter I do exactly this - assign the .private pointer and
> > > return "true." Next dma_chan_get() is called, which in turn calls driver's
> > > .device_alloc_chan_resources() method. There we check the previously set
> > > .private pointer to see, if this slave can be served by this DMA device.
> > > On sh-mobile you can freely configure single DMA channels for different
> > > slaves, as long as this slave is at all supported by the current DMA
> > > controller device. If this slave can be supported - all is good, we use
> > > the private data to configure the channel. If not - we return an error and
> > > __dma_request_channel() iterates to the next DMA controller device, which
> > > is exactly what we need.
> >
> > Haven't heard back from you after this my reply. Does this mean, that
> > you're now satisfied with my explanation and are going to apply my patch?
> >
> Sorry for the delay, we had a v long weekend here in India :)
> I am still not fully convinced yet. Why do you need the private pointer
> to be assigned in filter function? Once you return true for a channel in
> filter function, you should be able to use the channel properly.
> Assuming that you are calling for correct channel and you have set your
> private pointer (i don't understand for why/what), then above check
> seems to be bogus, why should you check again
> in .device_alloc_chan_resources?
That's exactly what I tried to explain above:
> > > On sh-mobile you can freely configure single DMA channels for different
> > > slaves, as long as this slave is at all supported by the current DMA
> > > controller device. If this slave can be supported - all is good, we use
> > > the private data to configure the channel. If not - we return an error and
> > > __dma_request_channel() iterates to the next DMA controller device, which
> > > is exactly what we need.
Let me try again. DMA channels on these DMA controllers are not dedicated.
On one such SoC there can be several such DMA controllers of different
kinds. One kind is "generic" - it can do memcpy(), besides channels can be
freely configured for one of onboard peripherals: serial, mmc, etc. Some
of them can also serve external DMA-capable devices. Another kind of DMA
controllers, served by the same driver, can only be used with USB
controllers. Now, if the MMC driver requests a DMA channel, let's say, the
dmaengine core first finds the USB DMA controller. The MMC driver cannot
know this. It assigns its MMC DMA configuration to the.private pointer and
returns true. Next the DMA driver is entered, it checks the private
pointer, sees an MMC channel request, looks at the DMA controller and
sees, that it doesn't support MMC. So, .device_alloc_chan_resources()
fails. When the same is attempted with a suitable DMA controller, the
shdma driver recognises, that the controller can service MMC and uses the
data, provided the MMC driver, to configure the DMA channel for MMC.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Vinod Koul @ 2011-09-05 13:25 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <Pine.LNX.4.64.1109051002400.1112@axis700.grange>
On Mon, 2011-09-05 at 10:04 +0200, Guennadi Liakhovetski wrote:
> Hi Vinod
>
> On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
>
> > On Tue, 30 Aug 2011, Vinod Koul wrote:
> >
> > > On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > > >
> > > > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > > > >> You should not assign chan->private.
> > > > > > > > >> Please move this to dma_slave_control API
> > > > > > > > >
> > > > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > > > I don't recall seeing any updated version fixing this
> > > > > >
> > > > > > As a matter of fact, when you say "use dma_slave_control API," you
> > > > > > actually mean the dma_slave_config, right? Then, how is one supposed to
> > > > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of
> > > > > > the filter and check the return code? The problem is, that not all DMA
> > > > > > controllers on sh-mobile SoCs can service the same slave devices. So, if
> > > > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > > > Here you are assigning to chan->private your specific values, which
> > > > > should be moved to the dma_slave_config.
> > > > > But here you are removing the checks, and accepting the first channel
> > > > > you got, so how do you find channel is suitable or not.
> > > >
> > > > That's done in the driver's .device_alloc_chan_resources() method. It
> > > > checkx the .private pointer, tries to find a suitable channel, if it fails
> > > > - it returns -EINVAL. See
> > > > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > > >
> > > > if (param) {
> > > > const struct sh_dmae_slave_config *cfg;
> > > >
> > > > cfg = sh_dmae_find_slave(sh_chan, param);
> > > > if (!cfg) {
> > > > ret = -EINVAL;
> > > > goto efindslave;
> > > > }
> > > Now am doubly confused. Are you saying that you are using
> > > alloc_chan_resources for doing filtering??
> >
> > Let's look at __dma_request_channel(). It iterates over DMA devices and
> > calls private_candidate() for each of them, which in turn calls the filter
> > function for each free channel on the current device. As soon as the
> > filter returns "true" for one of channels, a private candidate is found.
> > And in my filter I do exactly this - assign the .private pointer and
> > return "true." Next dma_chan_get() is called, which in turn calls driver's
> > .device_alloc_chan_resources() method. There we check the previously set
> > .private pointer to see, if this slave can be served by this DMA device.
> > On sh-mobile you can freely configure single DMA channels for different
> > slaves, as long as this slave is at all supported by the current DMA
> > controller device. If this slave can be supported - all is good, we use
> > the private data to configure the channel. If not - we return an error and
> > __dma_request_channel() iterates to the next DMA controller device, which
> > is exactly what we need.
>
> Haven't heard back from you after this my reply. Does this mean, that
> you're now satisfied with my explanation and are going to apply my patch?
>
Sorry for the delay, we had a v long weekend here in India :)
I am still not fully convinced yet. Why do you need the private pointer
to be assigned in filter function? Once you return true for a channel in
filter function, you should be able to use the channel properly.
Assuming that you are calling for correct channel and you have set your
private pointer (i don't understand for why/what), then above check
seems to be bogus, why should you check again
in .device_alloc_chan_resources?
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dma: shdma: transfer based runtime PM
From: Vinod Koul @ 2011-09-05 13:18 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: linux-kernel, linux-sh, Dan Williams, Paul Mundt
In-Reply-To: <Pine.LNX.4.64.1109051007230.1112@axis700.grange>
On Mon, 2011-09-05 at 10:10 +0200, Guennadi Liakhovetski wrote:
> Hi Vinod
>
> On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
Please dont top post...
> > Sorry, I see it differently. I don't use any counters in my patch. I'm
> > only checking for empty queue, i.e., I'm just identifying the first
> > descriptor submission and the last completion or termination.
> >
> > > You juts need
> > > to call _put/_get at right places, which IMO l;ooks lot simpler than
> > > current approach
> >
> > If we didn't have to check for exact symmetry, then yes, I agree, this
> > would be cleaner. I.e., if we indeed had well-defined entry- and
> > exit-points, which are guaranteed to be called exact same number of times.
> > Like, e.g., with file open() / close() etc. But since we don't have this
> > symmetry, and instead have to add flags and iterate lists, this doesn't
> > look natural and simple to me anymore, sorry.
>
> What about this one? Would you be prepared to take it as is, or you still
> think, that a pm_runtime_get*() on each descriptor submission would be
> better?
I think I will go with your current approach. Let me review again and
check it. If I get time it should be in my tree by tonight
--
~Vinod
^ permalink raw reply
* Re: [PATCH] dma: shdma: transfer based runtime PM
From: Guennadi Liakhovetski @ 2011-09-05 8:10 UTC (permalink / raw)
To: Vinod Koul; +Cc: linux-kernel, linux-sh, Dan Williams, Paul Mundt
In-Reply-To: <Pine.LNX.4.64.1108301321500.19151@axis700.grange>
Hi Vinod
On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> On Tue, 30 Aug 2011, Vinod Koul wrote:
>
> > On Tue, 2011-08-30 at 09:12 +0200, Guennadi Liakhovetski wrote:
> > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > Something like:
> > > > > > /* since callback is set for last descriptor of chain, we call runtime
> > > > > > * put for that desc alone
> > > > > > */
> > > > > > list_for_each_entry_safe(desc, __desc, sh_chan->ld_queue, node) {
> > > > > > if (desc->async_tx.callback)
> > > > > > pm_runtime_put(device);
> > > > >
> > > > > Not all dma users have callbacks.
> > > > Do you have such usage today, at least I dont :)
> > > > Nevertheless, in tx_submit adding a simple flag in your drivers
> > > > descriptor structure can tell you whether to call _put() or not. Agreed?
> > >
> > > Yes, I agree, that one could make this work too. Still, I do not
> > > understand how and why this is better to the extent, that I have to
> > > reimplement my patch, retest and resubmit it. Maybe Dan or Paul have an
> > > opinion on this?
> > But wont it make code look simpler and cleaner, you don't reply on your
> > counters but on pm_runtime infrastructure to do the job.
>
> Sorry, I see it differently. I don't use any counters in my patch. I'm
> only checking for empty queue, i.e., I'm just identifying the first
> descriptor submission and the last completion or termination.
>
> > You juts need
> > to call _put/_get at right places, which IMO l;ooks lot simpler than
> > current approach
>
> If we didn't have to check for exact symmetry, then yes, I agree, this
> would be cleaner. I.e., if we indeed had well-defined entry- and
> exit-points, which are guaranteed to be called exact same number of times.
> Like, e.g., with file open() / close() etc. But since we don't have this
> symmetry, and instead have to add flags and iterate lists, this doesn't
> look natural and simple to me anymore, sorry.
What about this one? Would you be prepared to take it as is, or you still
think, that a pm_runtime_get*() on each descriptor submission would be
better?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: don't filter on DMA device, use only
From: Guennadi Liakhovetski @ 2011-09-05 8:04 UTC (permalink / raw)
To: Vinod Koul
Cc: Magnus Damm, Paul Mundt, linux-sh, linux-kernel, Dan Williams,
Magnus Damm
In-Reply-To: <Pine.LNX.4.64.1108301332190.19151@axis700.grange>
Hi Vinod
On Tue, 30 Aug 2011, Guennadi Liakhovetski wrote:
> On Tue, 30 Aug 2011, Vinod Koul wrote:
>
> > On Tue, 2011-08-30 at 13:20 +0200, Guennadi Liakhovetski wrote:
> > > On Tue, 30 Aug 2011, Vinod Koul wrote:
> > >
> > > > On Tue, 2011-08-30 at 09:54 +0200, Guennadi Liakhovetski wrote:
> > > > > On Mon, 29 Aug 2011, Vinod Koul wrote:
> > > > > > > >> You should not assign chan->private.
> > > > > > > >> Please move this to dma_slave_control API
> > > > > > > >
> > > > > > > > I haven't seen any reply to this comment and this patch seems to still be
> > > > > > > > outstanding, is there an updated version of this patch that I've missed?
> > > > > > I don't recall seeing any updated version fixing this
> > > > >
> > > > > As a matter of fact, when you say "use dma_slave_control API," you
> > > > > actually mean the dma_slave_config, right? Then, how is one supposed to
> > > > > use it in this case? Shall we be issuing a DMA_SLAVE_CONFIG call inside of
> > > > > the filter and check the return code? The problem is, that not all DMA
> > > > > controllers on sh-mobile SoCs can service the same slave devices. So, if
> > > > > we don't check in the filter we might well get an unsuitable DMA channel.
> > > > Here you are assigning to chan->private your specific values, which
> > > > should be moved to the dma_slave_config.
> > > > But here you are removing the checks, and accepting the first channel
> > > > you got, so how do you find channel is suitable or not.
> > >
> > > That's done in the driver's .device_alloc_chan_resources() method. It
> > > checkx the .private pointer, tries to find a suitable channel, if it fails
> > > - it returns -EINVAL. See
> > > drivers/dma/shdma.c::sh_dmae_alloc_chan_resources():
> > >
> > > if (param) {
> > > const struct sh_dmae_slave_config *cfg;
> > >
> > > cfg = sh_dmae_find_slave(sh_chan, param);
> > > if (!cfg) {
> > > ret = -EINVAL;
> > > goto efindslave;
> > > }
> > Now am doubly confused. Are you saying that you are using
> > alloc_chan_resources for doing filtering??
>
> Let's look at __dma_request_channel(). It iterates over DMA devices and
> calls private_candidate() for each of them, which in turn calls the filter
> function for each free channel on the current device. As soon as the
> filter returns "true" for one of channels, a private candidate is found.
> And in my filter I do exactly this - assign the .private pointer and
> return "true." Next dma_chan_get() is called, which in turn calls driver's
> .device_alloc_chan_resources() method. There we check the previously set
> .private pointer to see, if this slave can be served by this DMA device.
> On sh-mobile you can freely configure single DMA channels for different
> slaves, as long as this slave is at all supported by the current DMA
> controller device. If this slave can be supported - all is good, we use
> the private data to configure the channel. If not - we return an error and
> __dma_request_channel() iterates to the next DMA controller device, which
> is exactly what we need.
Haven't heard back from you after this my reply. Does this mean, that
you're now satisfied with my explanation and are going to apply my patch?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 3/5] PM / QoS: Add function dev_pm_qos_read_value()
From: Jean Pihet @ 2011-09-05 7:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM mailing list, LKML, Linux-sh list, Magnus Damm,
Kevin Hilman
In-Reply-To: <201109031002.51487.rjw@sisk.pl>
Hi,
On Sat, Sep 3, 2011 at 10:02 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Hi,
>
> On Saturday, September 03, 2011, Rafael J. Wysocki wrote:
>> On Friday, September 02, 2011, Jean Pihet wrote:
>> > On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> ...
>> > >> >
>> > >> > - mutex_lock(&dev_pm_qos_mtx);
>> > >> > req->dev = dev;
>> > >> >
>> > >> > - /* Return if the device has been removed */
>> > >> > - if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
>> > >> > - ret = -ENODEV;
>> > >> > - goto out;
>> > >> > - }
>> > >> > + device_pm_lock();
>> > >> > + mutex_lock(&dev_pm_qos_mtx);
>> > >> >
>> > >> > - /*
>> > >> > - * Allocate the constraints data on the first call to add_request,
>> > >> > - * i.e. only if the data is not already allocated and if the device has
>> > >> > - * not been removed
>> > >> > - */
>> > >> > - if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
>> > >> > - ret = dev_pm_qos_constraints_allocate(dev);
>> > >> > + if (dev->power.constraints) {
>> > >> > + device_pm_unlock();
>> > >> > + } else {
>> > >> > + if (list_empty(&dev->power.entry)) {
>> > >> > + /* The device has been removed from the system. */
>> > >> > + device_pm_unlock();
>> > >> > + goto out;
>> > >> 0 is silently returned in case the device has been removed. Is that
>> > >> the intention?
>> > >
>> > > Pretty much it is. Is that a problem?
>> > I think the caller needs to know if the constraint has been applied
>> > correctly or not.
>>
>> However, the removal of the device is a special case. What would the caller
>> be supposed to do when it learned that the device had been removed while it had
>> been trying to add a QoS constraing for it? Not much I guess.
>
> Anyway, since returning an error code in that case won't hurt either,
> below is a v2 of the patch doing that and resetting the dev field in the
> req structure.
Ok thanks for the update. The comments are inlined below.
>
> Thanks,
> Rafael
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / QoS: Add function dev_pm_qos_read_value() (v2)
>
> To read the current PM QoS value for a given device we need to
> make sure that the device's power.constraints object won't be
> removed while we're doing that. For this reason, put the
> operation under dev->power.lock and acquire the lock
> around the initialization and removal of power.constraints.
>
> Moreover, since we're using the value of power.constraints to
> determine whether or not the object is present, the
> power.constraints_state field isn't necessary any more and may be
> removed. However, dev_pm_qos_add_request() needs to check if the
> device is being removed from the system before allocating a new
> PM QoS constraints object for it, so it has to use device_pm_lock()
> and the device PM QoS initialization and destruction should be done
> under device_pm_lock() as well.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> drivers/base/power/main.c | 4 -
> drivers/base/power/qos.c | 170 ++++++++++++++++++++++++++--------------------
> include/linux/pm.h | 8 --
> include/linux/pm_qos.h | 3
> 4 files changed, 104 insertions(+), 81 deletions(-)
>
> Index: linux/drivers/base/power/qos.c
> =================================> --- linux.orig/drivers/base/power/qos.c
> +++ linux/drivers/base/power/qos.c
> @@ -30,15 +30,6 @@
> * . To minimize the data usage by the per-device constraints, the data struct
> * is only allocated at the first call to dev_pm_qos_add_request.
> * . The data is later free'd when the device is removed from the system.
> - * . The constraints_state variable from dev_pm_info tracks the data struct
> - * allocation state:
> - * DEV_PM_QOS_NO_DEVICE: No device present or device removed, no data
> - * allocated,
> - * DEV_PM_QOS_DEVICE_PRESENT: Device present, data not allocated and will be
> - * allocated at the first call to dev_pm_qos_add_request,
> - * DEV_PM_QOS_ALLOCATED: Device present, data allocated. The per-device
> - * PM QoS constraints framework is operational and constraints can be
> - * added, updated or removed using the dev_pm_qos_* API.
> * . A global mutex protects the constraints users from the data being
> * allocated and free'd.
> */
> @@ -51,8 +42,30 @@
>
>
> static DEFINE_MUTEX(dev_pm_qos_mtx);
> +
> static BLOCKING_NOTIFIER_HEAD(dev_pm_notifiers);
>
> +/**
> + * dev_pm_qos_read_value - Get PM QoS constraint for a given device.
> + * @dev: Device to get the PM QoS constraint value for.
> + */
> +s32 dev_pm_qos_read_value(struct device *dev)
> +{
> + struct pm_qos_constraints *c;
> + unsigned long flags;
> + s32 ret = 0;
> +
> + spin_lock_irqsave(&dev->power.lock, flags);
> +
> + c = dev->power.constraints;
> + if (c)
> + ret = pm_qos_read_value(c);
> +
> + spin_unlock_irqrestore(&dev->power.lock, flags);
> +
> + return ret;
> +}
> +
> /*
> * apply_constraint
> * @req: constraint request to apply
> @@ -105,27 +118,37 @@ static int dev_pm_qos_constraints_alloca
> }
> BLOCKING_INIT_NOTIFIER_HEAD(n);
>
> + plist_head_init(&c->list);
> + c->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
> + c->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
> + c->type = PM_QOS_MIN;
> + c->notifiers = n;
> +
> + spin_lock_irq(&dev->power.lock);
> dev->power.constraints = c;
> - plist_head_init(&dev->power.constraints->list);
> - dev->power.constraints->target_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
> - dev->power.constraints->default_value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
> - dev->power.constraints->type = PM_QOS_MIN;
> - dev->power.constraints->notifiers = n;
> - dev->power.constraints_state = DEV_PM_QOS_ALLOCATED;
> + spin_unlock_irq(&dev->power.lock);
>
> return 0;
> }
>
> +static void __dev_pm_qos_constraints_init(struct device *dev)
> +{
> + spin_lock_irq(&dev->power.lock);
> + dev->power.constraints = NULL;
> + spin_unlock_irq(&dev->power.lock);
> +}
> +
> /**
> - * dev_pm_qos_constraints_init
> + * dev_pm_qos_constraints_init - Initalize device's PM QoS constraints pointer.
> * @dev: target device
> *
> - * Called from the device PM subsystem at device insertion
> + * Called from the device PM subsystem at device insertion under
> + * device_pm_lock().
> */
> void dev_pm_qos_constraints_init(struct device *dev)
> {
> mutex_lock(&dev_pm_qos_mtx);
> - dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT;
> + dev->power.constraints = NULL;
> mutex_unlock(&dev_pm_qos_mtx);
> }
>
> @@ -133,34 +156,35 @@ void dev_pm_qos_constraints_init(struct
> * dev_pm_qos_constraints_destroy
> * @dev: target device
> *
> - * Called from the device PM subsystem at device removal
> + * Called from the device PM subsystem at device removal under device_pm_lock().
> */
> void dev_pm_qos_constraints_destroy(struct device *dev)
> {
> struct dev_pm_qos_request *req, *tmp;
> + struct pm_qos_constraints *c;
>
> mutex_lock(&dev_pm_qos_mtx);
>
> - if (dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
> - /* Flush the constraints list for the device */
> - plist_for_each_entry_safe(req, tmp,
> - &dev->power.constraints->list,
> - node) {
> - /*
> - * Update constraints list and call the notification
> - * callbacks if needed
> - */
> - apply_constraint(req, PM_QOS_REMOVE_REQ,
> - PM_QOS_DEFAULT_VALUE);
> - memset(req, 0, sizeof(*req));
> - }
> + c = dev->power.constraints;
> + if (!c)
> + goto out;
>
> - kfree(dev->power.constraints->notifiers);
> - kfree(dev->power.constraints);
> - dev->power.constraints = NULL;
> + /* Flush the constraints list for the device */
> + plist_for_each_entry_safe(req, tmp, &c->list, node) {
> + /*
> + * Update constraints list and call the notification
> + * callbacks if needed
> + */
> + apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> + memset(req, 0, sizeof(*req));
> }
> - dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE;
>
> + __dev_pm_qos_constraints_init(dev);
> +
> + kfree(c->notifiers);
> + kfree(c);
> +
> + out:
> mutex_unlock(&dev_pm_qos_mtx);
> }
>
> @@ -178,8 +202,9 @@ void dev_pm_qos_constraints_destroy(stru
> *
> * Returns 1 if the aggregated constraint value has changed,
> * 0 if the aggregated constraint value has not changed,
> - * -EINVAL in case of wrong parameters, -ENODEV if the device has been
> - * removed from the system
> + * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
> + * to allocate for data structures, -ENODEV if the device has just been removed
> + * from the system.
> */
> int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
> s32 value)
> @@ -195,28 +220,37 @@ int dev_pm_qos_add_request(struct device
> return -EINVAL;
> }
>
> - mutex_lock(&dev_pm_qos_mtx);
> req->dev = dev;
>
> - /* Return if the device has been removed */
> - if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
> - ret = -ENODEV;
> - goto out;
> - }
> + device_pm_lock();
> + mutex_lock(&dev_pm_qos_mtx);
>
> - /*
> - * Allocate the constraints data on the first call to add_request,
> - * i.e. only if the data is not already allocated and if the device has
> - * not been removed
> - */
> - if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
> - ret = dev_pm_qos_constraints_allocate(dev);
> + if (dev->power.constraints) {
> + device_pm_unlock();
> + } else {
> + if (list_empty(&dev->power.entry)) {
> + /* The device has been removed from the system. */
> + device_pm_unlock();
> + req->dev = NULL;
> + ret = -ENODEV;
> + goto out;
> + } else {
> + device_pm_unlock();
> + /*
> + * Allocate the constraints data on the first call to
> + * add_request, i.e. only if the data is not already
> + * allocated and if the device has not been removed.
> + */
> + ret = dev_pm_qos_constraints_allocate(dev);
> + }
> + }
>
> if (!ret)
> ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
>
> -out:
> + out:
> mutex_unlock(&dev_pm_qos_mtx);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
> @@ -252,13 +286,13 @@ int dev_pm_qos_update_request(struct dev
>
> mutex_lock(&dev_pm_qos_mtx);
>
> - if (req->dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
> + if (req->dev->power.constraints) {
> if (new_value != req->node.prio)
> ret = apply_constraint(req, PM_QOS_UPDATE_REQ,
> new_value);
> } else {
> /* Return if the device has been removed */
> - ret = -ENODEV;
> + ret = -EINVAL;
The retcode should be -ENODEV. Is the kerneldoc still OK?
> }
>
> mutex_unlock(&dev_pm_qos_mtx);
> @@ -293,7 +327,7 @@ int dev_pm_qos_remove_request(struct dev
>
> mutex_lock(&dev_pm_qos_mtx);
>
> - if (req->dev->power.constraints_state = DEV_PM_QOS_ALLOCATED) {
> + if (req->dev->power.constraints) {
> ret = apply_constraint(req, PM_QOS_REMOVE_REQ,
> PM_QOS_DEFAULT_VALUE);
> memset(req, 0, sizeof(*req));
> @@ -323,15 +357,12 @@ int dev_pm_qos_add_notifier(struct devic
>
> mutex_lock(&dev_pm_qos_mtx);
>
> - /* Silently return if the device has been removed */
> - if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> - goto out;
> -
> - retval = blocking_notifier_chain_register(
> - dev->power.constraints->notifiers,
> - notifier);
> + /* Silently return if the constraints object is not present. */
> + if (dev->power.constraints)
> + retval = blocking_notifier_chain_register(
> + dev->power.constraints->notifiers,
> + notifier);
>
> -out:
> mutex_unlock(&dev_pm_qos_mtx);
> return retval;
> }
> @@ -354,15 +385,12 @@ int dev_pm_qos_remove_notifier(struct de
>
> mutex_lock(&dev_pm_qos_mtx);
>
> - /* Silently return if the device has been removed */
> - if (dev->power.constraints_state != DEV_PM_QOS_ALLOCATED)
> - goto out;
> -
> - retval = blocking_notifier_chain_unregister(
> - dev->power.constraints->notifiers,
> - notifier);
> + /* Silently return if the constraints object is not present. */
> + if (dev->power.constraints)
> + retval = blocking_notifier_chain_unregister(
> + dev->power.constraints->notifiers,
> + notifier);
>
> -out:
> mutex_unlock(&dev_pm_qos_mtx);
> return retval;
> }
> Index: linux/include/linux/pm_qos.h
> =================================> --- linux.orig/include/linux/pm_qos.h
> +++ linux/include/linux/pm_qos.h
> @@ -77,6 +77,7 @@ int pm_qos_remove_notifier(int pm_qos_cl
> int pm_qos_request_active(struct pm_qos_request *req);
> s32 pm_qos_read_value(struct pm_qos_constraints *c);
>
> +s32 dev_pm_qos_read_value(struct device *dev);
> int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
> s32 value);
> int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
> @@ -117,6 +118,8 @@ static inline int pm_qos_request_active(
> static inline s32 pm_qos_read_value(struct pm_qos_constraints *c)
> { return 0; }
>
> +static inline s32 dev_pm_qos_read_value(struct device *dev)
> + { return 0; }
> static inline int dev_pm_qos_add_request(struct device *dev,
> struct dev_pm_qos_request *req,
> s32 value)
> Index: linux/drivers/base/power/main.c
> =================================> --- linux.orig/drivers/base/power/main.c
> +++ linux/drivers/base/power/main.c
> @@ -98,8 +98,8 @@ void device_pm_add(struct device *dev)
> dev_warn(dev, "parent %s should not be sleeping\n",
> dev_name(dev->parent));
> list_add_tail(&dev->power.entry, &dpm_list);
> - mutex_unlock(&dpm_list_mtx);
> dev_pm_qos_constraints_init(dev);
> + mutex_unlock(&dpm_list_mtx);
> }
>
> /**
> @@ -110,9 +110,9 @@ void device_pm_remove(struct device *dev
> {
> pr_debug("PM: Removing info for %s:%s\n",
> dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
> - dev_pm_qos_constraints_destroy(dev);
> complete_all(&dev->power.completion);
> mutex_lock(&dpm_list_mtx);
> + dev_pm_qos_constraints_destroy(dev);
> list_del_init(&dev->power.entry);
> mutex_unlock(&dpm_list_mtx);
> device_wakeup_disable(dev);
> Index: linux/include/linux/pm.h
> =================================> --- linux.orig/include/linux/pm.h
> +++ linux/include/linux/pm.h
> @@ -421,13 +421,6 @@ enum rpm_request {
> RPM_REQ_RESUME,
> };
>
> -/* Per-device PM QoS constraints data struct state */
> -enum dev_pm_qos_state {
> - DEV_PM_QOS_NO_DEVICE, /* No device present */
> - DEV_PM_QOS_DEVICE_PRESENT, /* Device present, data not allocated */
> - DEV_PM_QOS_ALLOCATED, /* Device present, data allocated */
> -};
> -
> struct wakeup_source;
>
> struct pm_domain_data {
> @@ -489,7 +482,6 @@ struct dev_pm_info {
> #endif
> struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */
> struct pm_qos_constraints *constraints;
> - enum dev_pm_qos_state constraints_state;
> };
>
> extern void update_pm_runtime_accounting(struct device *dev);
>
Thanks,
Jean
^ permalink raw reply
* Re: [PATCH 3/5] PM / QoS: Add function dev_pm_qos_read_value()
From: Jean Pihet @ 2011-09-05 7:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM mailing list, LKML, Linux-sh list, Magnus Damm,
Kevin Hilman
In-Reply-To: <201109030155.28031.rjw@sisk.pl>
Hi Rafael,
On Sat, Sep 3, 2011 at 1:55 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, September 02, 2011, Jean Pihet wrote:
>> On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
...
>> >> > */
>> >> > int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
>> >> > s32 value)
>> >> > @@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
>> >> > return -EINVAL;
>> >> > }
>> >> >
>> >> > - mutex_lock(&dev_pm_qos_mtx);
>> >> > req->dev = dev;
>> >> >
>> >> > - /* Return if the device has been removed */
>> >> > - if (req->dev->power.constraints_state = DEV_PM_QOS_NO_DEVICE) {
>> >> > - ret = -ENODEV;
>> >> > - goto out;
>> >> > - }
>> >> > + device_pm_lock();
>> >> > + mutex_lock(&dev_pm_qos_mtx);
>> >> >
>> >> > - /*
>> >> > - * Allocate the constraints data on the first call to add_request,
>> >> > - * i.e. only if the data is not already allocated and if the device has
>> >> > - * not been removed
>> >> > - */
>> >> > - if (dev->power.constraints_state = DEV_PM_QOS_DEVICE_PRESENT)
>> >> > - ret = dev_pm_qos_constraints_allocate(dev);
>> >> > + if (dev->power.constraints) {
>> >> > + device_pm_unlock();
>> >> > + } else {
>> >> > + if (list_empty(&dev->power.entry)) {
>> >> > + /* The device has been removed from the system. */
>> >> > + device_pm_unlock();
>> >> > + goto out;
>> >> 0 is silently returned in case the device has been removed. Is that
>> >> the intention?
>> >
>> > Pretty much it is. Is that a problem?
>> I think the caller needs to know if the constraint has been applied
>> correctly or not.
>
> However, the removal of the device is a special case. What would the caller
> be supposed to do when it learned that the device had been removed while it had
> been trying to add a QoS constraing for it? Not much I guess.
When the device is removed from the system the constraints list id
flushed and the requests are zero'ed using memset.
In that case the caller needs to be aware of that otherwise the next
call to the API will throw an error.
>
> Thanks,
> Rafael
>
Thanks,
Jean
^ permalink raw reply
* Re: Ecovec in current sh-latest
From: Guennadi Liakhovetski @ 2011-09-05 7:38 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <Pine.LNX.4.64.1109020025350.15166@axis700.grange>
On Mon, 5 Sep 2011, Paul Mundt wrote:
> On Fri, Sep 02, 2011 at 10:17:54AM +0200, Guennadi Liakhovetski wrote:
> > To get ecovec to run with today's sh-latest (pretty much the same with
> > Linux' tree) the following patches have to be reverted:
> >
> > commit 794d78fea51504bad3880d14f354a9847f318f25
> > Author: Magnus Damm <damm@opensource.se>
> > Date: Tue Jun 21 07:55:12 2011 +0000
> >
> > drivers: sh: late disabling of clocks V2
> >
> Do we know which driver that's the problem for the ecovec case?
I see at least two drivers misbehaving: ethernet is failing to perform
dhcp for nfs root-mount, and LCD gets an image at boot and then slowly
fade out.
> We can of course pile on another CLK_ENABLE_ON_INIT workaround for 3.1,
> so long as there's a plan to get the driver properly fixed for 3.2.
>
> > commit 3f9b8520b06013939ad247ba08b69529b5f14be1
> > Author: Paul Mundt <lethal@linux-sh.org>
> > Date: Tue May 31 14:38:29 2011 +0900
> >
> > sh64: Move from P1SEG to CAC_ADDR for consistent sync.
> >
> I'll queue a revert for this for 3.1. There's quite a bit of bogosity
> going on in some of the drivers that this exposed, but we're not going to
> get it all fixed in one shot, either.
>
> > commit cb3da5b837a410a1fb90019549b18178f3c87258
> > Author: Magnus Damm <damm@opensource.se>
> > Date: Wed Aug 3 12:47:36 2011 +0900
> >
> > serial: sh-sci: console Runtime PM support
> >
> What's the issue with this one? I'm assuming this and the clock disabling
> revert are related.
IIRC, the boot process stops after
console [ttySC0] enabled
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 1/2] SPI: spi_sh_msiof: implement DMA support
From: Guennadi Liakhovetski @ 2011-09-05 7:26 UTC (permalink / raw)
To: Paul Mundt; +Cc: spi-devel-general, linux-sh, Grant Likely
In-Reply-To: <20110905045911.GE22142@linux-sh.org>
Hi Paul
On Mon, 5 Sep 2011, Paul Mundt wrote:
> On Fri, Sep 02, 2011 at 05:13:31PM +0200, Guennadi Liakhovetski wrote:
> > Use the sh_dma dmaengine driver to support DMA on MSIOF.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>
> No real opinion one way or the other, just a couple observations.
>
> > +static void sh_msiof_request_dma(struct sh_msiof_spi_priv *p)
> > +{
> ..
> > + p->dummypage = alloc_page(GFP_KERNEL);
> > + if (!p->dummypage)
> > + return;
> > +
> ..
> > +echantx:
> > + __free_pages(p->dummypage, 0);
> > +}
> > +
> alloc_page() can be balanced out with __free_page().
Hm, indeed, wondering, how I missed it.
> > @@ -695,11 +1030,11 @@ static int sh_msiof_spi_remove(struct platform_device *pdev)
> >
> > ret = spi_bitbang_stop(&p->bitbang);
> > if (!ret) {
> > + sh_msiof_release_dma(p);
> > pm_runtime_disable(&pdev->dev);
> > free_irq(platform_get_irq(pdev, 0), p);
> > iounmap(p->mapbase);
> > clk_put(p->clk);
> > - spi_master_put(p->bitbang.master);
> > }
> > return ret;
> > }
>
> You've also killed off the spi_master_put() here.
Yes, spi_bitbang_stop() does it all already.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 0/2] mmc: sh_mmcif: simplify platform DMA configuration
From: Guennadi Liakhovetski @ 2011-09-05 7:15 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <Pine.LNX.4.64.1108301819150.19151@axis700.grange>
On Mon, 5 Sep 2011, Paul Mundt wrote:
> On Tue, Aug 30, 2011 at 06:26:34PM +0200, Guennadi Liakhovetski wrote:
> > A simple cosmetic clean-up, no functional changes. Patch 2/2 depends on
> > patch 1/2 and can wait until 3.3. Paul, would you be able to put it under
> > the carpet somewhere until then or shall I resend it after 3.2-rc1 is out?
> > After both these patches have been applied, we can remove struct
> > sh_mmcif_plat_data::dma around 3.4 or 4.0 or whatever;-)
> >
> It's not a problem, I'll just flag it as awaiting upstream in the
> tracker. Anything flagged as such I generally give a once over after each
> merge window to see if their dependencies have been worked out.
Good, thanks!
Regards
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 00/03] ARM: mach-shmobile: sh7372 power domain update
From: Paul Mundt @ 2011-09-05 5:46 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <20110630094653.10442.68817.sendpatchset@t400s>
On Fri, Aug 26, 2011 at 03:43:16PM +0900, Magnus Damm wrote:
> ARM: mach-shmobile: sh7372 power domain update
>
> [PATCH 01/03] ARM: mach-shmobile: sh7372 A3SM support
> [PATCH 02/03] ARM: mach-shmobile: sh7372 A3SP support
> [PATCH 03/03] ARM: mach-shmobile: sh7372 A4R support
>
> These patches add support for the A3SM, A3SP and A4R
> power domains included in the sh7372 SoC. More work
> in needed with QoS for I/O devices and CPUIdle with
> A3SM together with future A4S power domain support.
>
> Testing needed, especiall WRT to Suspend-to-RAM!
>
> Signed-off-by: Magnus Damm <damm@opensource.se>
As these don't apply against anything I have I'll assume that Rafael will
be taking them through the pm tree with the rest of the power domains
code.
^ permalink raw reply
* Re: [PATCH 5/7] fbdev: sh_mipi_dsi: add extra dcs settings for platform
From: Paul Mundt @ 2011-09-05 5:33 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <87vctmt9t1.wl%kuninori.morimoto.gx@renesas.com>
On Mon, Aug 29, 2011 at 10:45:37AM +0200, Guennadi Liakhovetski wrote:
> On Mon, 29 Aug 2011, Magnus Damm wrote:
> > I do however wonder if this array is the best approach to adding board
> > specific code. My personal take would be to use something like the SYS
> > bus support for the LCDC, see callbacks in struct
> > sh_mobile_lcdc_sys_bus_ops located in
> > include/video/sh_mobile_lcdc.h and board/panel code like
> > arch/sh/boards/mach-migor/lcd_qvga.c.
>
> I don't like that array either, but if we want to do this _right_,
> callbacks wouldn't be my preference either. I'm not familiar with the set
> up: are those values wpecific to the LCD panel or to the board? If it's
> the LCD panel, then ideally one would want a display driver for them.
> Backlight controlling would want a proper driver too.
>
> > Also, if your board/panel specific code is dealing with backlight on
> > MIPI-DSI, perhaps there is a chance this is generic enough to be
> > broken out and reused somehow.
>
> Exactly, however, lately I'me coming to an unfortunate conclusion, that
> due to a vast variety of embedded systems and their components, many
> hardware blocks will ever be only used on a single embedded platform,
> running Linux, and only be exposed to a handful of users. So, creating and
> maintaining a proper driver for each such block has a good chance to be
> considered an overkill...
>
That's true, but I'm not sure I see how that matters for the case at
hand. MIPI is hardly SH-Mobile specific, it just happens to be the place
where a lot of the work is being done for the moment (OMAP DSS being
another). There is already a lot of duplication between the different
MIPI users in-tree today, and it's reasonable to expect that sooner or
later there will be some consolidation effort.
Given that all of the SH-Mobile parts are also using it going forward (at
least for the moment) it's certainly worth trying to get right. Throwing
on some opaque "extras" pointer at the end of the structure is just
asking for abuse, particularly if the only purpose it's serving is more
or less common functionality we can expect to see on upcoming platforms.
While the SYS bus bits were pretty much stillborn, this wasn't so much a
problem of the abstraction (or lack thereof -- and I'm certainly not
endorsing the SYS bus abstraction, either) as general timing. At the time
most of that work happened it was still playing catch up to support a
platform that would be obsolete by the time support was sufficiently far
enough for anyone to make use of it. The present line of SH-Mobile CPUs
don't really suffer from the same sort of knee-jerk IP block scizophrenia
usually associated with the device team (even if it seems like we're
sometimes suffering from CPU or board of the week syndrome), so I would
certainly lean towards doing it right, even if it's going to take a bit
longer to hash out the abstraction.
Your comments about a proper backlight driver and so on are accurate as
well, and this is something we were also discussing with the SYS bus
bits, it simply wasn't bothered with since the platform was basically
abandoned before anyone really cared.
^ permalink raw reply
* Re: [PATCH] [RFC] sh: kexec: Register crashk_res
From: Simon Horman @ 2011-09-05 5:30 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1314935232-1545-1-git-send-email-horms@verge.net.au>
On Mon, Sep 05, 2011 at 01:36:27PM +0900, Paul Mundt wrote:
> On Mon, Sep 05, 2011 at 01:25:50PM +0900, Simon Horman wrote:
> > On Mon, Sep 05, 2011 at 12:23:53PM +0900, Paul Mundt wrote:
> > > On Fri, Sep 02, 2011 at 12:47:12PM +0900, Simon Horman wrote:
> > > > Register crashk_res so that it can be used by kexec-tools
> > > > via /proc/iomem.
> > > >
> > > > On x86 the registration occurs using
> > > > insert_resource(&iomem_resource, &crashk_res).
> > > > However that approach seems to result in the boot hanging on SH.
> > > >
> > > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > >
> > > x86 has a slightly more straightforward registration method. We end up
> > > going through the same path for all memory ranges, which also
> > > encapsulates the NUMA case. As such, we don't necessarily know which
> > > range will contain the resource in question, so it's attempted on each
> > > range addition, expecting the resource manager to work things out for us.
> > >
> > > With the request_resource() in place you presumably see the crash kernel
> > > resource where you expect it to in /proc/iomem?
> >
> > Yes. With this patch in place the crash kernel resource
> > shows up in /proc/iomem in a sensible way.
> >
> > # cat /proc/iomem
> > 40000000-4effffff : System RAM
> > 40001000-402dbfc7 : Kernel code
> > 402dbfc8-403cd51f : Kernel data
> > 40553000-40568c5b : Kernel bss
> > 4d000000-4effffff : Crash kernel
> > ...
>
> Ok, I've queued it up with a slightly more verbose changelog. I'll send
> it along for 3.1 so kexec-tools doesn't remain blocked.
Thanks.
^ permalink raw reply
* Re: Ecovec in current sh-latest
From: Paul Mundt @ 2011-09-05 5:06 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <Pine.LNX.4.64.1109020025350.15166@axis700.grange>
On Fri, Sep 02, 2011 at 10:17:54AM +0200, Guennadi Liakhovetski wrote:
> To get ecovec to run with today's sh-latest (pretty much the same with
> Linux' tree) the following patches have to be reverted:
>
> commit 794d78fea51504bad3880d14f354a9847f318f25
> Author: Magnus Damm <damm@opensource.se>
> Date: Tue Jun 21 07:55:12 2011 +0000
>
> drivers: sh: late disabling of clocks V2
>
Do we know which driver that's the problem for the ecovec case?
We can of course pile on another CLK_ENABLE_ON_INIT workaround for 3.1,
so long as there's a plan to get the driver properly fixed for 3.2.
> commit 3f9b8520b06013939ad247ba08b69529b5f14be1
> Author: Paul Mundt <lethal@linux-sh.org>
> Date: Tue May 31 14:38:29 2011 +0900
>
> sh64: Move from P1SEG to CAC_ADDR for consistent sync.
>
I'll queue a revert for this for 3.1. There's quite a bit of bogosity
going on in some of the drivers that this exposed, but we're not going to
get it all fixed in one shot, either.
> commit cb3da5b837a410a1fb90019549b18178f3c87258
> Author: Magnus Damm <damm@opensource.se>
> Date: Wed Aug 3 12:47:36 2011 +0900
>
> serial: sh-sci: console Runtime PM support
>
What's the issue with this one? I'm assuming this and the clock disabling
revert are related.
^ permalink raw reply
* Re: [PATCH 1/2] SPI: spi_sh_msiof: implement DMA support
From: Paul Mundt @ 2011-09-05 4:59 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: spi-devel-general, linux-sh, Grant Likely
In-Reply-To: <Pine.LNX.4.64.1109021712180.4731@axis700.grange>
On Fri, Sep 02, 2011 at 05:13:31PM +0200, Guennadi Liakhovetski wrote:
> Use the sh_dma dmaengine driver to support DMA on MSIOF.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
No real opinion one way or the other, just a couple observations.
> +static void sh_msiof_request_dma(struct sh_msiof_spi_priv *p)
> +{
..
> + p->dummypage = alloc_page(GFP_KERNEL);
> + if (!p->dummypage)
> + return;
> +
..
> +echantx:
> + __free_pages(p->dummypage, 0);
> +}
> +
alloc_page() can be balanced out with __free_page().
> @@ -695,11 +1030,11 @@ static int sh_msiof_spi_remove(struct platform_device *pdev)
>
> ret = spi_bitbang_stop(&p->bitbang);
> if (!ret) {
> + sh_msiof_release_dma(p);
> pm_runtime_disable(&pdev->dev);
> free_irq(platform_get_irq(pdev, 0), p);
> iounmap(p->mapbase);
> clk_put(p->clk);
> - spi_master_put(p->bitbang.master);
> }
> return ret;
> }
You've also killed off the spi_master_put() here.
^ permalink raw reply
* Re: [PATCH] [RFC] sh: kexec: Register crashk_res
From: Paul Mundt @ 2011-09-05 4:36 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1314935232-1545-1-git-send-email-horms@verge.net.au>
On Mon, Sep 05, 2011 at 01:25:50PM +0900, Simon Horman wrote:
> On Mon, Sep 05, 2011 at 12:23:53PM +0900, Paul Mundt wrote:
> > On Fri, Sep 02, 2011 at 12:47:12PM +0900, Simon Horman wrote:
> > > Register crashk_res so that it can be used by kexec-tools
> > > via /proc/iomem.
> > >
> > > On x86 the registration occurs using
> > > insert_resource(&iomem_resource, &crashk_res).
> > > However that approach seems to result in the boot hanging on SH.
> > >
> > > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > x86 has a slightly more straightforward registration method. We end up
> > going through the same path for all memory ranges, which also
> > encapsulates the NUMA case. As such, we don't necessarily know which
> > range will contain the resource in question, so it's attempted on each
> > range addition, expecting the resource manager to work things out for us.
> >
> > With the request_resource() in place you presumably see the crash kernel
> > resource where you expect it to in /proc/iomem?
>
> Yes. With this patch in place the crash kernel resource
> shows up in /proc/iomem in a sensible way.
>
> # cat /proc/iomem
> 40000000-4effffff : System RAM
> 40001000-402dbfc7 : Kernel code
> 402dbfc8-403cd51f : Kernel data
> 40553000-40568c5b : Kernel bss
> 4d000000-4effffff : Crash kernel
> ...
Ok, I've queued it up with a slightly more verbose changelog. I'll send
it along for 3.1 so kexec-tools doesn't remain blocked.
^ permalink raw reply
* Re: [PATCH] [RFC] sh: kexec: Register crashk_res
From: Simon Horman @ 2011-09-05 4:25 UTC (permalink / raw)
To: linux-sh
In-Reply-To: <1314935232-1545-1-git-send-email-horms@verge.net.au>
On Mon, Sep 05, 2011 at 12:23:53PM +0900, Paul Mundt wrote:
> On Fri, Sep 02, 2011 at 12:47:12PM +0900, Simon Horman wrote:
> > Register crashk_res so that it can be used by kexec-tools
> > via /proc/iomem.
> >
> > On x86 the registration occurs using
> > insert_resource(&iomem_resource, &crashk_res).
> > However that approach seems to result in the boot hanging on SH.
> >
> > Signed-off-by: Simon Horman <horms@verge.net.au>
>
> x86 has a slightly more straightforward registration method. We end up
> going through the same path for all memory ranges, which also
> encapsulates the NUMA case. As such, we don't necessarily know which
> range will contain the resource in question, so it's attempted on each
> range addition, expecting the resource manager to work things out for us.
>
> With the request_resource() in place you presumably see the crash kernel
> resource where you expect it to in /proc/iomem?
Yes. With this patch in place the crash kernel resource
shows up in /proc/iomem in a sensible way.
# cat /proc/iomem
40000000-4effffff : System RAM
40001000-402dbfc7 : Kernel code
402dbfc8-403cd51f : Kernel data
40553000-40568c5b : Kernel bss
4d000000-4effffff : Crash kernel
...
^ permalink raw reply
* Re: [PATCH] ARM: mach-shmobile: ag5evm needs CONFIG_I2C
From: Paul Mundt @ 2011-09-05 4:19 UTC (permalink / raw)
To: linux-sh
On Tue, Aug 30, 2011 at 06:19:13PM +0200, Guennadi Liakhovetski wrote:
> ag5evm implements a backlight control, using an I2C controller, therefore
> it needs CONFIG_I2C to fix this make failure
>
> arch/arm/mach-shmobile/built-in.o: In function `lcd_on':
> pfc-sh73a0.c:(.text+0x2334): undefined reference to `i2c_get_adapter'
> pfc-sh73a0.c:(.text+0x2370): undefined reference to `i2c_transfer'
>
> (ignore pfc-sh73a0.c) and to build successfully.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Applied, thanks.
^ 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