Re: [PATCH v2 1/4] ARM: dts: omap5: Update GPIO with address space and interrupts
Hi Seb,
On 10/23/2012 03:37 AM, Sebastien Guiriec wrote:
Add base address and interrupt line inside Device Tree data for
OMAP5
Signed-off-by: Sebastien Guiriec s-guir...@ti.com
---
arch/arm/boot/dts/omap5.dtsi | 16
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 42c78be..9e39f9f 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -104,6 +104,8 @@
gpio1: gpio@4ae1 {
compatible = ti,omap4-gpio;
+ reg = 0x4ae1 0x200;
+ interrupts = 0 29 0x4;
ti,hwmods = gpio1;
gpio-controller;
#gpio-cells = 2;
I am wondering if we should add the interrupt-parent property to add
nodes in the device-tree source. I know that today the interrupt-parent
is being defined globally, but when device-tree maps an interrupt for a
device it searches for the interrupt-parent starting the current device
node.
So in other words, for gpio1 it will search the gpio1 binding for
interrupt-parent and if not found move up a level and search again. It
will keep doing this until it finds the interrupt-parent.
Therefore, I believe it will improve search time and hence, boot time if
we have interrupt-parent defined in each node.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] ARM: dts: omap5: Update GPIO with address space and interrupts
On 10/23/2012 10:09 AM, Benoit Cousson wrote:
On 10/23/2012 04:49 PM, Jon Hunter wrote:
Hi Seb,
On 10/23/2012 03:37 AM, Sebastien Guiriec wrote:
Add base address and interrupt line inside Device Tree data for
OMAP5
Signed-off-by: Sebastien Guiriec s-guir...@ti.com
---
arch/arm/boot/dts/omap5.dtsi | 16
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/omap5.dtsi b/arch/arm/boot/dts/omap5.dtsi
index 42c78be..9e39f9f 100644
--- a/arch/arm/boot/dts/omap5.dtsi
+++ b/arch/arm/boot/dts/omap5.dtsi
@@ -104,6 +104,8 @@
gpio1: gpio@4ae1 {
compatible = ti,omap4-gpio;
+ reg = 0x4ae1 0x200;
+ interrupts = 0 29 0x4;
ti,hwmods = gpio1;
gpio-controller;
#gpio-cells = 2;
I am wondering if we should add the interrupt-parent property to add
nodes in the device-tree source. I know that today the interrupt-parent
is being defined globally, but when device-tree maps an interrupt for a
device it searches for the interrupt-parent starting the current device
node.
So in other words, for gpio1 it will search the gpio1 binding for
interrupt-parent and if not found move up a level and search again. It
will keep doing this until it finds the interrupt-parent.
Therefore, I believe it will improve search time and hence, boot time if
we have interrupt-parent defined in each node.
Mmm, I'm not that sure. it will increase the size of the blob, so
increase the time to load it and then to parse it. Where in the current
case, it is just going up to the parent node using the already
un-flatten tree in memory and thus that should not take that much time.
Yes it will definitely increase the size, so that could slow things down.
That being said, it might be interesting to benchmark that to see what
is the real impact.
Right, I wonder what the key functions are we need to benchmark to get
an overall feel for what is best? Right now I am seeing some people add
the interrupt-parent for device nodes and others not. Ideally we should
be consistent, but at the same time it is probably something that we can
easily sort out later. So not a big deal either way.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/4] ARM: dts: omap5: Update GPIO with address space and interrupts
Hi Mitch, On 10/23/2012 11:55 AM, Mitch Bradley wrote: On 10/23/2012 4:49 AM, Jon Hunter wrote: Therefore, I believe it will improve search time and hence, boot time if we have interrupt-parent defined in each node. I strongly suspect (based on many years of performance tuning, with special focus on boot time) that the time difference will be completely insignificant. The total extra time for walking up the interrupt tree for every interrupt in a large system is comparable to the time it takes to send a few characters out a UART. So you can get more improvement from eliminating a single printk() than from globally adding per-node interrupt-parent. Furthermore, the cost of processing all of the interrupt-parent properties is probably similar to the cost of the avoided tree walks. CPU cycles are very fast compared to I/O register accesses, say a factor of 100. Now consider that many modern devices contain embedded microcontrollers (SD cards, network interface modules, USB hubs and devices, ...), and those devices usually require various delays measured in milliseconds, to ensure that the microcontroller is ready for the next initialization step. Those delays are extremely long compared to CPU cycles. Obviously, some of that can be overlapped by careful multithreading, but that isn't free either. The bottom line is that I'm pretty sure that adding per-node interrupt-parent would not be worthwhile from the standpoint of speeding up boot time. Absolutely, I don't expect this to miraculously improve the boot time or suggest that this is a major contributor to boot time, but what is the best approach in general in terms of efficiency (memory and time). In other words, is there a best practice? And from your feedback, I understand that adding a global interrupt-parent is a good practice. For a bit of fun, I took an omap4430 board and benchmarked the time taken by the of_irq_find_parent() when interrupt-parent was defined for each node using interrupts and without. There were a total of 47 device nodes using interrupts. Adding the interrupt-parent to all 47 nodes increased the dtb from 13211 bytes to 13963 bytes. On boot-up I saw 117 calls to of_irq_find_parent() for this platform (there appears to be multiple calls for a given device). Without interrupt-parent defined for each node total time spent in of_irq_find_parent() was 1.028 ms where as with interrupt-parent defined for each node the total time was 0.4032 ms. This was done using a 38.4MHz timer and the overhead of reading the timer 117 times was about 36 us. I understand that this does not provide the full picture, but I wanted to get a better handle on the times here. So yes the overall overhead here is not significant for us to worry about. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: discrepancy while save and restore of debounce registers
Hi Gururaja,
On 10/17/2012 01:13 AM, Hebbar, Gururaja wrote:
Hi,
I came across a peculiar issue while updating GPIO debounce registers on
OMAP platform.
According to mainline commit ae547354a8ed59f19b57f7e1de9c7816edfc3537
gpio/omap: save and restore debounce registers
GPIO debounce registers need to be saved and restored for proper functioning
of driver.
...
@@ -1363,6 +1369,12 @@ static void omap_gpio_restore_context(struct gpio_bank
*bank)
__raw_writel(bank-context.fallingdetect,
bank-base + bank-regs-fallingdetect);
__raw_writel(bank-context.dataout, bank-base + bank-regs-dataout);
+ if (bank-dbck_enable_mask) {
+ __raw_writel(bank-context.debounce, bank-base +
+ bank-regs-debounce);
+ __raw_writel(bank-context.debounce_en,
+ bank-base + bank-regs-debounce_en);
+ }
}
Due to copy/paste of this commit into my local tree, I missed the check for
bank-dbck_enable_mask, and directly restored the saved value from context.
After this, I saw random crashes when accessing different registers (sometimes
its OE register and sometime its DATAOUT register).
These crashes were seen across 2nd and subsequent suspend/resume.
My doubt/questions are
1. Why should debounce registers be updated only when it's accessed
previously?
If debounce is not being used by any of the gpios, then there is no need
to restore them as there are no bits set. So this makes sense and saves
a couple register writes.
2. What is the relation between updating debounce registers and crash seen on
others registers?
This I am not sure about. I gave this a quick try on my omap3430 beagle
board, but I did not see any side-effects from doing this. However, if
you are always restoring the debounce context regardless of whether
debounce is being used, then you could be writing bad values to the
debounce registers as the context variables bank-context.debouce and
bank-context.debouce_en may not initialised. So that is bad. However,
that said I am still not sure how this could cause a crash.
Can you share more details on ...
1. The OMAP platform you are using?
2. What linux distro/environment you are using?
3. If there are any specific steps to reproduce this 100% of the time?
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: discrepancy while save and restore of debounce registers
Hi Gururaja, On 10/18/2012 12:31 AM, Hebbar, Gururaja wrote: Jon, On Thu, Oct 18, 2012 at 02:42:01, Hunter, Jon wrote: [snip] My doubt/questions are 1. Why should debounce registers be updated only when it's accessed previously? If debounce is not being used by any of the gpios, then there is no need to restore them as there are no bits set. So this makes sense and saves a couple register writes. What I want to know is that other than saving register writes, is there any other important stuff that specifies this requirement. From a HW perspective, none that I can see. From a SW perspective, yes, as I mentioned if you restore these registers without first initialising the context variables where these registers are stored, then you may be restoring unknown values to the registers and that is bad. 2. What is the relation between updating debounce registers and crash seen on others registers? This I am not sure about. I gave this a quick try on my omap3430 beagle board, but I did not see any side-effects from doing this. However, if you are always restoring the debounce context regardless of whether debounce is being used, then you could be writing bad values to the debounce registers as the context variables bank-context.debouce and bank-context.debouce_en may not initialised. So that is bad. However, that said I am still not sure how this could cause a crash. Can you share more details on ... Sorry for missing below details in first post. 1. The OMAP platform you are using? I was trying this on TI AM335x platform (repo below). On AM335x EVM board http://arago-project.org/git/projects/?p=linux-am33x.git;a=shortlog; h=refs/heads/v3.2-staging 2. What linux distro/environment you are using? Arago AM335x PSP release (linux 3.2 + am335x patch-set) 3. If there are any specific steps to reproduce this 100% of the time? On top of this tree, try suspend/resume using echo mem /syspower/state I have a beagle-bone but unfortunately, suspend does not appear to be supported in the mainline kernel yet so I am unable to test this on the am335x on the mainline. Have you compared the gpio driver from your v3.2 branch with the current mainline to see how different this code is? Ideally, it would be good to test on the mainline kernel for another data point to see if this is local to your branch. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: dts: omap4-panda: Add LED support into the panda DTS files
On 04/17/2013 02:09 PM, Dan Murphy wrote: The GPIO for LED D1 on the omap4-panda a1-a3 rev and the omap4-panda-es are different. Abstract away the pinmux and the LED definitions for the two boards. Just a heads-up but you should base this upon Benoit's for_3.10 branch [1] as there is now a omap4-panda-common.dtsi where the led stuff currently resides. Cheers Jon [1] http://git.kernel.org/cgit/linux/kernel/git/bcousson/linux-omap-dt.git/log/?h=for_3.10/dts -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] dma: of: Fix of_node reference leak
On 04/19/2013 05:10 AM, Arnd Bergmann wrote: On Friday 19 April 2013, Lars-Peter Clausen wrote: of_dma_request_slave_channel() currently does not drop the reference to the dma_spec of_node if no DMA controller matching the of_node could be found. This patch fixes it by always calling of_node_put(). Signed-off-by: Lars-Peter Clausen l...@metafoo.de Acked-by: Arnd Bergmann a...@arndb.de Thanks! FWIW ... Reviewed-by: Jon Hunter jon-hun...@ti.com Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma:of: Use a mutex to protect the of_dma_list
On 04/19/2013 04:42 AM, Lars-Peter Clausen wrote:
Currently the OF DMA code uses a spin lock to protect the of_dma_list from
concurrent access and a per controller reference count to protect the
controller
from being freed while a request operation is in progress. If
of_dma_controller_free() is called for a controller who's reference count is
not
zero it will return -EBUSY and not remove the controller. This is fine up
until
here, but leaves the question what the caller of of_dma_controller_free() is
supposed to do if the controller couldn't be freed. The only viable solution
for the caller is to spin on of_dma_controller_free() until it returns
success.
E.g.
do {
ret = of_dma_controller_free(dev-of_node)
} while (ret != -EBUSY);
This is rather ugly and unnecessary and non of the current users of
of_dma_controller_free() check it's return value anyway. Instead protect the
list by a mutex. The mutex will be held as long as a request operation is in
progress. So if of_dma_controller_free() is called while a request operation
is
in progress it will be put to sleep and only wake up once the request
operation
has finished.
This means that it is no longer possible to register or unregister OF DMA
controllers from a context where it's not possible to sleep. But I doubt that
we'll ever need this.
Change also means that of_dma_request_slave_channel() cannot be called
from a context where it is not possible to sleep too, right? May be
worth mentioning this in the changelog as well.
Also rename of_dma_get_controller back to of_dma_find_controller.
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
drivers/dma/of-dma.c | 76
+-
include/linux/of_dma.h | 6 ++--
2 files changed, 22 insertions(+), 60 deletions(-)
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 2882403..7aa0864 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -13,38 +13,31 @@
#include linux/device.h
#include linux/err.h
#include linux/module.h
-#include linux/rculist.h
+#include linux/mutex.h
#include linux/slab.h
#include linux/of.h
#include linux/of_dma.h
static LIST_HEAD(of_dma_list);
-static DEFINE_SPINLOCK(of_dma_lock);
+static DEFINE_MUTEX(of_dma_lock);
/**
- * of_dma_get_controller - Get a DMA controller in DT DMA helpers list
+ * of_dma_find_controller - Get a DMA controller in DT DMA helpers list
* @dma_spec:pointer to DMA specifier as found in the device tree
*
* Finds a DMA controller with matching device node and number for dma cells
- * in a list of registered DMA controllers. If a match is found the use_count
- * variable is increased and a valid pointer to the DMA data stored is
retuned.
- * A NULL pointer is returned if no match is found.
+ * in a list of registered DMA controllers. If a match is found a valid
pointer
+ * to the DMA data stored is retuned. A NULL pointer is returned if no match
is
+ * found.
*/
-static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec)
+static struct of_dma *of_dma_find_controller(struct of_phandle_args
*dma_spec)
{
struct of_dma *ofdma;
- spin_lock(of_dma_lock);
-
list_for_each_entry(ofdma, of_dma_list, of_dma_controllers)
if ((ofdma-of_node == dma_spec-np)
- (ofdma-of_dma_nbcells == dma_spec-args_count)) {
- ofdma-use_count++;
- spin_unlock(of_dma_lock);
+ (ofdma-of_dma_nbcells == dma_spec-args_count))
return ofdma;
- }
-
- spin_unlock(of_dma_lock);
pr_debug(%s: can't find DMA controller %s\n, __func__,
dma_spec-np-full_name);
@@ -53,22 +46,6 @@ static struct of_dma *of_dma_get_controller(struct
of_phandle_args *dma_spec)
}
/**
- * of_dma_put_controller - Decrement use count for a registered DMA
controller
- * @of_dma: pointer to DMA controller data
- *
- * Decrements the use_count variable in the DMA data structure. This function
- * should be called only when a valid pointer is returned from
- * of_dma_get_controller() and no further accesses to data referenced by that
- * pointer are needed.
- */
-static void of_dma_put_controller(struct of_dma *ofdma)
-{
- spin_lock(of_dma_lock);
- ofdma-use_count--;
- spin_unlock(of_dma_lock);
-}
-
-/**
* of_dma_controller_register - Register a DMA controller to DT DMA helpers
* @np: device node of DMA controller
* @of_dma_xlate:translation function which converts a phandle
@@ -114,12 +91,11 @@ int of_dma_controller_register(struct device_node *np,
ofdma-of_dma_nbcells = nbcells;
ofdma-of_dma_xlate = of_dma_xlate;
ofdma-of_dma_data = data;
- ofdma-use_count = 0;
/* Now queue of_dma controller structure in list */
-
Re: [PATCH 2/2] dma:of: Use a mutex to protect the of_dma_list
On 04/20/2013 05:38 AM, Arnd Bergmann wrote: On Saturday 20 April 2013, Lars-Peter Clausen wrote: On 04/20/2013 12:45 AM, Jon Hunter wrote: I think that there is a problem here. For controllers using the of_dma_simple_xlate(), this will call dma_request_channel() which also uses a mutex. That would only be a problem if it'd use the same mutex. Holding two mutexes at the same time is not a problem per se. I guess Jon originlly tried it with a spinlock as the outer lock, which indeed does not work, but the new version does not have this problem. Yes the spinlock was a problem and hence for the put/get functions. Ok so that's fine then. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma:of: Use a mutex to protect the of_dma_list
On 04/19/2013 06:13 PM, Arnd Bergmann wrote: On Saturday 20 April 2013, Jon Hunter wrote: Change also means that of_dma_request_slave_channel() cannot be called from a context where it is not possible to sleep too, right? May be worth mentioning this in the changelog as well. You already cannot do that, because it requires dma_list_mutex. Right in the case where you use of_dma_simple_xlate(). However, someone could implement whatever they wanted for the xlate. Probably not likely as it needs to return struct dma_chan, but could be possible. That's all. Not a big deal. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: video: omap2: dss: Use PTR_RET function
On 03/20/2013 06:56 AM, Tomi Valkeinen wrote: On 2013-03-19 10:03, Alexandru Gheorghiu wrote: Use PTR_RET function instead of IS_ERR and PTR_ERR. Patch found using coccinelle. Signed-off-by: Alexandru Gheorghiu gheorghiuan...@gmail.com --- drivers/video/omap2/dss/core.c |5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c index f8779d4..60cc6fe 100644 --- a/drivers/video/omap2/dss/core.c +++ b/drivers/video/omap2/dss/core.c @@ -181,10 +181,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *)) d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir, write, dss_debug_fops); -if (IS_ERR(d)) -return PTR_ERR(d); - -return 0; +return PTR_RET(d); } #else /* CONFIG_OMAP2_DSS_DEBUGFS */ static inline int dss_initialize_debugfs(void) Thanks. I'll apply to omapdss tree. Is this correct? If debugfs_create_file() returns a valid pointer, then now dss_debugfs_create_file() will return a non-zero value on success. I don't think this is what you want. A similar case came up recently here [1]. Jon [1] http://comments.gmane.org/gmane.linux.kernel/1455141 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: video: omap2: dss: Use PTR_RET function
On 03/20/2013 11:59 AM, Tomi Valkeinen wrote: On 2013-03-20 17:44, Jon Hunter wrote: On 03/20/2013 06:56 AM, Tomi Valkeinen wrote: On 2013-03-19 10:03, Alexandru Gheorghiu wrote: Use PTR_RET function instead of IS_ERR and PTR_ERR. Patch found using coccinelle. Signed-off-by: Alexandru Gheorghiu gheorghiuan...@gmail.com --- drivers/video/omap2/dss/core.c |5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c index f8779d4..60cc6fe 100644 --- a/drivers/video/omap2/dss/core.c +++ b/drivers/video/omap2/dss/core.c @@ -181,10 +181,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *)) d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir, write, dss_debug_fops); - if (IS_ERR(d)) - return PTR_ERR(d); - - return 0; + return PTR_RET(d); } #else /* CONFIG_OMAP2_DSS_DEBUGFS */ static inline int dss_initialize_debugfs(void) Thanks. I'll apply to omapdss tree. Is this correct? If debugfs_create_file() returns a valid pointer, then now dss_debugfs_create_file() will return a non-zero value on success. I don't think this is what you want. A similar case came up recently here [1]. Hmm. I don't follow. And I don't understand the post where you referred. PTR_RET is defined as: if (IS_ERR(ptr)) return PTR_ERR(ptr); else return 0; How's that different from the original code? Oops sorry, I missed that you were returning PTR_RET() and not PTR_ERR(). Yes that looks fine. Sorry for the noise! Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: video: omap2: dss: Use PTR_RET function
On 03/20/2013 01:02 PM, Jon Hunter wrote: On 03/20/2013 11:59 AM, Tomi Valkeinen wrote: On 2013-03-20 17:44, Jon Hunter wrote: On 03/20/2013 06:56 AM, Tomi Valkeinen wrote: On 2013-03-19 10:03, Alexandru Gheorghiu wrote: Use PTR_RET function instead of IS_ERR and PTR_ERR. Patch found using coccinelle. Signed-off-by: Alexandru Gheorghiu gheorghiuan...@gmail.com --- drivers/video/omap2/dss/core.c |5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c index f8779d4..60cc6fe 100644 --- a/drivers/video/omap2/dss/core.c +++ b/drivers/video/omap2/dss/core.c @@ -181,10 +181,7 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *)) d = debugfs_create_file(name, S_IRUGO, dss_debugfs_dir, write, dss_debug_fops); - if (IS_ERR(d)) - return PTR_ERR(d); - - return 0; + return PTR_RET(d); } #else /* CONFIG_OMAP2_DSS_DEBUGFS */ static inline int dss_initialize_debugfs(void) Thanks. I'll apply to omapdss tree. Is this correct? If debugfs_create_file() returns a valid pointer, then now dss_debugfs_create_file() will return a non-zero value on success. I don't think this is what you want. A similar case came up recently here [1]. Hmm. I don't follow. And I don't understand the post where you referred. Yes looking at Russell's response I am not sure I following now as it is also using PTR_RET() and not PTR_ERR(). My eyes deceived me on this one. Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mach_omap2: use PTR_RET instead of IS_ERR + PTR_ERR
On 03/12/2013 06:05 AM, Russell King - ARM Linux wrote:
On Tue, Mar 12, 2013 at 09:58:29AM +0200, Silviu-Mihai Popescu wrote:
This uses PTR_RET instead of IS_ERR and PTR_ERR in order to increase
readability.
Signed-off-by: Silviu-Mihai Popescu silviupopescu1...@gmail.com
---
arch/arm/mach-omap2/devices.c |4 ++--
arch/arm/mach-omap2/fb.c |5 +
arch/arm/mach-omap2/gpmc.c|2 +-
arch/arm/mach-omap2/pmu.c |5 +
4 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 1ec7f05..2a0816e 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -66,7 +66,7 @@ static int __init omap3_l3_init(void)
WARN(IS_ERR(pdev), could not build omap_device for %s\n, oh_name);
-return IS_ERR(pdev) ? PTR_ERR(pdev) : 0;
+return PTR_RET(pdev);
This is incorrect.
The return value will be tested for 0. Kernel pointers in general are
all above 3GB, and so are all 0.
I'm afraid none of these changes stuff is an improvement - they all
introduce bugs.
Sorry I am now not sure I follow you here. Someone just pointed out to
me that PTR_RET() is defined as ...
static inline int __must_check PTR_RET(const void *ptr)
{
if (IS_ERR(ptr))
return PTR_ERR(ptr);
else
return 0;
}
So the above change appears to be equivalent. Is there something that is
wrong with the current implementation that needs to be fixed?
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mach_omap2: use PTR_RET instead of IS_ERR + PTR_ERR
On 03/22/2013 11:36 AM, Russell King - ARM Linux wrote:
On Wed, Mar 20, 2013 at 01:28:47PM -0500, Jon Hunter wrote:
Sorry I am now not sure I follow you here. Someone just pointed out to
me that PTR_RET() is defined as ...
static inline int __must_check PTR_RET(const void *ptr)
{
if (IS_ERR(ptr))
return PTR_ERR(ptr);
else
return 0;
}
So the above change appears to be equivalent. Is there something that is
wrong with the current implementation that needs to be fixed?
No - I misread it as PTR_ERR not PTR_RET. Your patch is fine.
Thanks for confirming. I had made the same mistake recently too!
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/3] mmc: omap_hsmmc: Skip platform_get_resource_byname() for dt case
On 03/06/2013 07:56 AM, Matt Porter wrote: On Wed, Mar 06, 2013 at 07:12:29PM +0530, Balaji T K wrote: On Wednesday 06 March 2013 02:43 AM, Matt Porter wrote: From: Santosh Shilimkar santosh.shilim...@ti.com MMC driver probe will abort for DT case because of failed platform_get_resource_byname() lookup. Fix it by skipping resource byname lookup for device tree build. Issue is hidden because hwmod popullates the IO resources which helps to succeed platform_get_resource_byname() and probe. Hi Matt, Could you please drop this patch from the current series, since this patch causes regression on omap3,4 platform which are not yet dma dt adapted. It is best to send this patch along with Jon Hunter dma dt series, which adds dt dma support and mmc dma data. DMA dt series is needed any way before hwmod cleanup can happen. *sigh* ok, I should have never split this stuff out from the am33xx series. Will do. There will not be any regression if all of these make the same merge window. I am hoping that the omap dma patches will make 3.10 as well. I think that it is best for Matt to keep his patches together although now I see he has already posted a V3 dropping this :-( Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 09/11] mfd: twl-core: Collect global variables behind one private structure (global)
On 01/16/2013 07:53 AM, Peter Ujfalusi wrote:
Gather the global variables under a single structure and allocate it with
devm_kzalloc(). It is easier to see them and if in the future we try to add
support for multiple instance of twl in the system it is going to be much
simpler.
Signed-off-by: Peter Ujfalusi peter.ujfal...@ti.com
---
drivers/mfd/twl-core.c | 104
+++--
1 file changed, 57 insertions(+), 47 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 1827088..e2895a4 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -141,33 +141,28 @@
/*--*/
-/* is driver active, bound to a chip? */
-static bool inuse;
-
-/* TWL IDCODE Register value */
-static u32 twl_idcode;
-
-static unsigned int twl_id;
-unsigned int twl_rev(void)
-{
- return twl_id;
-}
-EXPORT_SYMBOL(twl_rev);
-
/* Structure for each TWL4030/TWL6030 Slave */
struct twl_client {
struct i2c_client *client;
struct regmap *regmap;
};
-static struct twl_client *twl_modules;
-
/* mapping the module id to slave id and base address */
struct twl_mapping {
unsigned char sid; /* Slave ID */
unsigned char base; /* base address */
};
-static struct twl_mapping *twl_map;
+
+struct twl_private {
+ bool ready; /* The core driver is ready to be used */
+ u32 twl_idcode; /* TWL IDCODE Register value */
+ unsigned int twl_id;
+
+ struct twl_mapping *twl_map;
+ struct twl_client *twl_modules;
+};
+
+static struct twl_private *twl_priv;
static struct twl_mapping twl4030_map[] = {
/*
@@ -300,6 +295,12 @@ static inline int twl_get_last_module(void)
/* Exported Functions */
+unsigned int twl_rev(void)
+{
+ return twl_priv ? twl_priv-twl_id : 0;
+}
+EXPORT_SYMBOL(twl_rev);
+
/**
* twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
* @mod_no: module number
@@ -322,16 +323,17 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg,
unsigned num_bytes)
pr_err(%s: invalid module number %d\n, DRIVER_NAME, mod_no);
return -EPERM;
}
- if (unlikely(!inuse)) {
+ if (unlikely(!twl_priv-ready)) {
This is causing the kernel to panic on all my omap2 boards when booting
linux-next
because twl_priv is not initialised yet.
pr_err(%s: not initialized\n, DRIVER_NAME);
return -EPERM;
}
- sid = twl_map[mod_no].sid;
- twl = twl_modules[sid];
+ sid = twl_priv-twl_map[mod_no].sid;
+ twl = twl_priv-twl_modules[sid];
- ret = regmap_bulk_write(twl-regmap, twl_map[mod_no].base + reg,
- value, num_bytes);
+ ret = regmap_bulk_write(twl-regmap,
+ twl_priv-twl_map[mod_no].base + reg, value,
+ num_bytes);
if (ret)
pr_err(%s: Write failed (mod %d, reg 0x%02x count %d)\n,
@@ -360,16 +362,17 @@ int twl_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned
num_bytes)
pr_err(%s: invalid module number %d\n, DRIVER_NAME, mod_no);
return -EPERM;
}
- if (unlikely(!inuse)) {
+ if (unlikely(!twl_priv-ready)) {
Same problem here.
Here is a fix ...
From 141fcbbdee6bdc14d5a444ff20fad6b3440215dc Mon Sep 17 00:00:00 2001
From: Jon Hunter jon-hun...@ti.com
Date: Fri, 8 Feb 2013 12:42:20 -0600
Subject: [PATCH] ARM: OMAP2+: Fix kernel panic on boot
Commit 8a6aaa3 (mfd: twl-core: Collect global variables behind one
private structure (global)) removed the variable inuse that is used
to determine if the device has been initialised and now use the
twl_priv structure instead. This is causing the kernel to panic on all
OMAP2+ devices, because we try to access the twl_priv-ready member
before checking if twl_priv is initialised. Fix this and move this test
to the beginning of the twl_i2c_read/write function because
twl_get_last_module() also uses the twl_priv structure.
Signed-off-by: Jon Hunter jon-hun...@ti.com
---
drivers/mfd/twl-core.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 557f9ee..89ab4d9 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -316,12 +316,12 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned
num_bytes)
int sid;
struct twl_client *twl;
- if (unlikely(mod_no = twl_get_last_module())) {
- pr_err(%s: invalid module number %d\n, DRIVER_NAME, mod_no);
+ if (unlikely(!twl_priv || !twl_priv-ready)) {
+ pr_err(%s: not initialized\n, DRIVER_NAME);
return -EPERM;
}
- if (unlikely(!twl_priv-ready)) {
- pr_err(%s: not initialized\n, DRIVER_NAME);
+ if (unlikely(mod_no
Re: [PATCH 1/1] ARM: OMAP4: Add OMAP4 Blaze Tablet support
On 02/09/2013 08:52 PM, Tony Lindgren wrote: Hi, * Ruslan Bilovol ruslan.bilo...@ti.com [130208 11:41]: The OMAP4 Blaze Tablet is TI OMAP4 processor-based development platform in a tablet formfactor. The platform contains many of the features found in present-day handsets (such as audio, video, wireless functions and user interfaces) and in addition contains features for software development and test. This patch adds initial support for the OMAP4 Blaze Tablet development platform. Additional functionality depends on different drivers and code modifications that are not upstreamed yet so will be added later. Nice that you have it working, however, we're not adding any more board-*.c files as we're moving things to device tree based booting. Care to try to get the basic .dts file done for this? You will probably need to add CONFIG_ARM_APPENDED_DTB=y and CONFIG_ARM_ATAG_DTB_COMPAT=y to omap2plus_defconfig and append the .dtb file to zImage and run mkimage then manually to boot it. But you should get serial and MMC at least working to start with :) Please note that the blaze is derived from the omap4-sdp board and so I would hope that we can use the existing for sdp dts and board file for blaze. In fact this is what I do today for basic booting. So unless there is some feature on the blaze that is not compatible with the original sdp, we should just use the sdp dts. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 09/11] mfd: twl-core: Collect global variables behind one private structure (global)
On 02/08/2013 11:50 PM, Peter Ujfalusi wrote:
On 02/08/2013 07:56 PM, Jon Hunter wrote:
/**
* twl_i2c_write - Writes a n bit register in TWL4030/TWL5030/TWL60X0
* @mod_no: module number
@@ -322,16 +323,17 @@ int twl_i2c_write(u8 mod_no, u8 *value, u8 reg,
unsigned num_bytes)
pr_err(%s: invalid module number %d\n, DRIVER_NAME, mod_no);
return -EPERM;
}
- if (unlikely(!inuse)) {
+ if (unlikely(!twl_priv-ready)) {
This is causing the kernel to panic on all my omap2 boards when booting
linux-next
because twl_priv is not initialised yet.
Good catch.
I just wonder from where the twl_* call is coming on OMAP2. AFAIK the twl code
is for OMAP3/4, for OMAP2 Menelaus is the one which is used.
I'm currently working on to remove all those twl_* calls from random places in
the kernel so we will only access twl via the MFD stack.
Good point. I just noticed that none of my omap2+ board were booting and
on omap3/4 I was the panic in the twl code. I can't say that I checked
the panic on omap2, so may be that was another problem?
I will update the changelog and re-send the patch.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 09/11] mfd: twl-core: Collect global variables behind one private structure (global)
On 02/12/2013 01:26 AM, Peter Ujfalusi wrote: On 02/11/2013 09:22 PM, Jon Hunter wrote: Good point. I just noticed that none of my omap2+ board were booting and on omap3/4 I was the panic in the twl code. I can't say that I checked the panic on omap2, so may be that was another problem? Do you have insights on the code path leading to a crash on OMAP3/4? I have been running this code on several boards (BeagleBoard, Zoom2, PandaBoard, Blaze) and have not seen a crash. Here is the panic log ... [2.286132] Unable to handle kernel NULL pointer dereference at virtual address [2.294738] pgd = c0004000 [2.297576] [] *pgd= [2.301361] Internal error: Oops: 5 [#1] SMP ARM [2.306243] Modules linked in: [2.309448] CPU: 0Not tainted (3.8.0-rc6-next-20130207-00016-g735c237 #35) [2.317169] PC is at twl_i2c_read+0x3c/0xec [2.321563] LR is at twl_i2c_read+0x1c/0xec [2.325988] pc : [c0333950]lr : [c0333930]psr: 8153 [2.325988] sp : c702fed0 ip : fp : [2.338043] r10: c702e000 r9 : c06e84e8 r8 : c06e51c8 [2.343536] r7 : 0001 r6 : 0006 r5 : c702fef6 r4 : 0004 [2.350402] r3 : c129508c r2 : 0006 r1 : c702fef6 r0 : 000e [2.357269] Flags: Nzcv IRQs on FIQs off Mode SVC_32 ISA ARM Segment kernel [2.365051] Control: 10c5387d Table: 80004019 DAC: 0017 [2.371093] Process swapper/0 (pid: 1, stack limit = 0xc702e240) [2.377410] Stack: (0xc702fed0 to 0xc703) [2.382019] fec0: c0d42180 c0d429d0 c70a7640 c07354c4 [2.390624] fee0: 0001 c0719798 c0d42180 c06f2cc0 c04e76cc c06ee1ac c07354c4 [2.399230] ff00: 0007 c06f2d64 0017 c06fb308 c06f07a4 c0cb8580 c06edee0 [2.407836] ff20: c06eded4 c06e8504 c0d42180 c0008768 009e c00611b4 0001 [2.416442] ff40: c061994c c06b7548 0007 0007 c07354c4 0007 c0719798 [2.425048] ff60: c0d42180 c06e51c8 c07197a0 009e c06e590c 0007 0007 [2.433654] ff80: c06e51c8 c04d26ec [2.442260] ffa0: c04d26f4 c00137b0 [2.450866] ffc0: [2.459472] ffe0: 0013 80fb6c10 71bbcd20 [2.468078] [c0333950] (twl_i2c_read+0x3c/0xec) from [c06f2cc0] (omap3_twl_set_sr_bit+0x3c/0xb4) [2.477722] [c06f2cc0] (omap3_twl_set_sr_bit+0x3c/0xb4) from [c06f2d64] (omap3_twl_init+0x2c/0x70) [2.487518] [c06f2d64] (omap3_twl_init+0x2c/0x70) from [c06fb308] (omap_pmic_late_init+0x18/0x24) [2.497222] [c06fb308] (omap_pmic_late_init+0x18/0x24) from [c06f07a4] (omap2_common_pm_late_init+0x18/0xd0) [2.507934] [c06f07a4] (omap2_common_pm_late_init+0x18/0xd0) from [c06edee0] (omap3_init_late+0xc/0x18) [2.518188] [c06edee0] (omap3_init_late+0xc/0x18) from [c06e8504] (init_machine_late+0x1c/0x28) [2.527740] [c06e8504] (init_machine_late+0x1c/0x28) from [c0008768] (do_one_initcall+0x100/0x16c) [2.537536] [c0008768] (do_one_initcall+0x100/0x16c) from [c06e590c] (kernel_init_freeable+0xfc/0x1c8) [2.547698] [c06e590c] (kernel_init_freeable+0xfc/0x1c8) from [c04d26f4] (kernel_init+0x8/0xe4) [2.557250] [c04d26f4] (kernel_init+0x8/0xe4) from [c00137b0] (ret_from_fork+0x14/0x24) But the fix is valid. Thanks. I saw this on linux-next earlier this week, but now I am not seeing it and the fix I posted is not there. So I am not sure what changed to cause this to occur earlier this week but the problem appears to be gone again. However, at least the fix will prevent such panics if someone is calling twl_i2c_read/write too early. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] arm: dts: omap4-panda: Add I2c pinctrl data
On 02/13/2013 03:28 AM, Sourav Poddar wrote:
Booting 3.8-rc6 on omap4 panda results in the following error
[0.27] omap_i2c 4807.i2c: did not get pins for i2c error: -19
[0.445770] omap_i2c 4807.i2c: bus 0 rev0.11 at 400 kHz
[0.473937] omap_i2c 48072000.i2c: did not get pins for i2c error: -19
[0.474670] omap_i2c 48072000.i2c: bus 1 rev0.11 at 400 kHz
[0.474822] omap_i2c 4806.i2c: did not get pins for i2c error: -19
[0.476379] omap_i2c 4806.i2c: bus 2 rev0.11 at 100 kHz
[0.477294] omap_i2c 4835.i2c: did not get pins for i2c error: -19
[0.477996] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz
[0.483398] Switching to clocksource 32k_counter
This happens because omap4 panda dts file is not adapted to use i2c through
pinctrl framework. Populating i2c pinctrl data to get rid of the error.
What about the panda-es and panda-a4?
Tested on omap4460 panda with 3.8-rc6 kernel.
Signed-off-by: Sourav Poddar sourav.pod...@ti.com
Reported-by: Luciano Coelho coe...@ti.com
---
arch/arm/boot/dts/omap4-panda.dts | 40
+
1 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/omap4-panda.dts
b/arch/arm/boot/dts/omap4-panda.dts
index 4122efe..f951e6b 100644
--- a/arch/arm/boot/dts/omap4-panda.dts
+++ b/arch/arm/boot/dts/omap4-panda.dts
@@ -110,9 +110,40 @@
0x58 0x10b /* hdmi_hpd.gpio_63 INPUT PULLDOWN |
MODE3 */
;
};
+
+ i2c1_pins: pinmux_i2c1_pins {
+ pinctrl-single,pins =
+ 0xe2 0x118/* i2c1_scl PULLUP | INPUTENABLE |
MODE0 */
+ 0xe4 0x118/* i2c1_sda PULLUP | INPUTENABLE |
MODE0 */
+ ;
+ };
+
+ i2c2_pins: pinmux_i2c2_pins {
+ pinctrl-single,pins =
+ 0xe6 0x118/* i2c2_scl PULLUP | INPUTENABLE |
MODE0 */
+ 0xe8 0x118/* i2c2_sda PULLUP | INPUTENABLE |
MODE0 */
+ ;
+ };
+
+ i2c3_pins: pinmux_i2c3_pins {
+ pinctrl-single,pins =
+ 0xea 0x118/* i2c3_scl PULLUP | INPUTENABLE |
MODE0 */
+ 0xec 0x118 /* i2c3_sda PULLUP | INPUTENABLE | MODE0
*/
+ ;
+ };
+
+ i2c4_pins: pinmux_i2c4_pins {
+ pinctrl-single,pins =
+ 0xee 0x118/* i2c4_scl PULLUP | INPUTENABLE |
MODE0 */
+ 0xf0 0x118 /* i2c4_sda PULLUP | INPUTENABLE | MODE0
*/
+ ;
+ };
};
A quick look at the data manual shows that omap4430 and omap4460 has the
same pin mux options for i2c. Furthermore, the data manual shows only
one mux option for i2c1-4. Therefore, should these mux options be placed
in omap4.dtsi? Boards not using specific i2c controllers can disabled
them in there board dts file (same way we do for mmc).
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/5] arm: dts: omap4-panda: Add I2c pinctrl data
On 02/13/2013 09:57 AM, Jon Hunter wrote:
On 02/13/2013 03:28 AM, Sourav Poddar wrote:
Booting 3.8-rc6 on omap4 panda results in the following error
[0.27] omap_i2c 4807.i2c: did not get pins for i2c error: -19
[0.445770] omap_i2c 4807.i2c: bus 0 rev0.11 at 400 kHz
[0.473937] omap_i2c 48072000.i2c: did not get pins for i2c error: -19
[0.474670] omap_i2c 48072000.i2c: bus 1 rev0.11 at 400 kHz
[0.474822] omap_i2c 4806.i2c: did not get pins for i2c error: -19
[0.476379] omap_i2c 4806.i2c: bus 2 rev0.11 at 100 kHz
[0.477294] omap_i2c 4835.i2c: did not get pins for i2c error: -19
[0.477996] omap_i2c 4835.i2c: bus 3 rev0.11 at 400 kHz
[0.483398] Switching to clocksource 32k_counter
This happens because omap4 panda dts file is not adapted to use i2c through
pinctrl framework. Populating i2c pinctrl data to get rid of the error.
What about the panda-es and panda-a4?
Tested on omap4460 panda with 3.8-rc6 kernel.
Signed-off-by: Sourav Poddar sourav.pod...@ti.com
Reported-by: Luciano Coelho coe...@ti.com
---
arch/arm/boot/dts/omap4-panda.dts | 40
+
1 files changed, 40 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/omap4-panda.dts
b/arch/arm/boot/dts/omap4-panda.dts
index 4122efe..f951e6b 100644
--- a/arch/arm/boot/dts/omap4-panda.dts
+++ b/arch/arm/boot/dts/omap4-panda.dts
@@ -110,9 +110,40 @@
0x58 0x10b /* hdmi_hpd.gpio_63 INPUT PULLDOWN |
MODE3 */
;
};
+
+i2c1_pins: pinmux_i2c1_pins {
+pinctrl-single,pins =
+0xe2 0x118/* i2c1_scl PULLUP | INPUTENABLE |
MODE0 */
+0xe4 0x118/* i2c1_sda PULLUP | INPUTENABLE |
MODE0 */
+;
+};
+
+i2c2_pins: pinmux_i2c2_pins {
+pinctrl-single,pins =
+0xe6 0x118/* i2c2_scl PULLUP | INPUTENABLE |
MODE0 */
+0xe8 0x118/* i2c2_sda PULLUP | INPUTENABLE |
MODE0 */
+;
+};
+
+i2c3_pins: pinmux_i2c3_pins {
+pinctrl-single,pins =
+0xea 0x118/* i2c3_scl PULLUP | INPUTENABLE |
MODE0 */
+0xec 0x118 /* i2c3_sda PULLUP | INPUTENABLE | MODE0
*/
+;
+};
+
+i2c4_pins: pinmux_i2c4_pins {
+pinctrl-single,pins =
+0xee 0x118/* i2c4_scl PULLUP | INPUTENABLE |
MODE0 */
+0xf0 0x118 /* i2c4_sda PULLUP | INPUTENABLE | MODE0
*/
+;
+};
};
A quick look at the data manual shows that omap4430 and omap4460 has the
same pin mux options for i2c. Furthermore, the data manual shows only
one mux option for i2c1-4. Therefore, should these mux options be placed
in omap4.dtsi? Boards not using specific i2c controllers can disabled
them in there board dts file (same way we do for mmc).
I guess for i2c, a given omap4 board may use external pull-ups and not
use the internal ones and so putting this in the omap4.dtsi may not be
desirable. However, it seems that a common omap4-panda.dtsi could be
used here.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: OMAP4: Add OMAP4 Blaze Tablet support
On 02/13/2013 05:28 PM, Ruslan Bilovol wrote: Hi Tony, Jon, On Mon, Feb 11, 2013 at 9:00 PM, Tony Lindgren t...@atomide.com wrote: * Jon Hunter jon-hun...@ti.com [130211 10:58]: Please note that the blaze is derived from the omap4-sdp board and so I would hope that we can use the existing for sdp dts and board file for blaze. In fact this is what I do today for basic booting. So unless there is some feature on the blaze that is not compatible with the original sdp, we should just use the sdp dts. Sounds like we need some common .dts file and separate files for sdp, blaze and tablet that include the common .dts file? There's a different LCD panel at least between blaze and the tablet. Please note, that, although 'Blaze' board is very close to 'SDP' board, there are quite big differences comparing to 'Blaze Tablet' board. At least: - LCD panels - touchscreen controllers - LEDs - Keypad (on 'Blaze') / gpio keys (on 'Blaze Tablet') - Sensors - HS USB Host related stuff SDP also has usb host too (but yes blaze does not). - cameras (there is no embedded cameras on 'Blaze Tablet' board) As per my point of view, next should be done: 1) Add the DTS file for BlazeTablet board 2) Figure out what is common for both and move it to some common file (if that makes sense) I'm currently working on step '#1'. So after that step #2 should not be an issue for us. Sounds good. They all use the same processor boards and so may be that can be common in DT and we can have a dtsi for that. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CoreSight framework and drivers
On 12/19/2012 03:24 PM, Pratik Patel wrote: [snip] Currently we use the CoreSight virtual bus to conveniently list sysfs configuration attributes for all the registered CoreSight devices. For eg: /sys/bus/coresight/devices/coresight-etm0/attribute /sys/bus/coresight/devices/coresight-etm1/attribute /sys/bus/coresight/devices/coresight-stm/attribute /sys/bus/coresight/devices/coresight-tmc-etf/attribute ... ... Some of the attributes are based on device type (i.e. source, link or sink) so they will exist for all devices of that type while some are device specific. Maybe I am misunderstanding the question but are you suggesting to register CoreSight devices to the AMBA bus instead of the CoreSight core layer code? Yes exactly. Will Deacon mentioned earlier that AMBA framework can be changed to accomodate devices with a different class but I am more concerned with losing some of the stuff that the core layer code does (eg. coresight_register, coresight_enable, coresight_disable in coresight.c) if we register CoreSight drivers to the AMBA bus without letting the core layer know about the device connections. I may be missing something, but couldn't you keep all the register/enable/disable stuff but just register the device with the amba bus? Obviously some changes would need to be made. Personally, I don't have strong feeling either way, but we have ETM/ETB drivers using AMBA today and so I am hoping we can come to agreement on this going forward. Russell, Will, what are your thoughts? Otherwise, looking at the code, I like what you have implemented. I still need to look closer, but I am struggling to figure out how a coresight device such as the cross-trigger-interface fits with this model. This model appears to be geared towards coresight devices used for traces purposes and are either source, links or sinks. The cross-trigger-interface is not a source or a sink. However, although you it could be considered as a link (routing events), it is not really, as it may not link to other coresight sinks/source. In my case, I have PMU-IRQ -- CTI -- GIC. So a non-coresight source and sink. In away the CTI looks more like a 2nd-level interrupt controller than anything else. Hence, another type of coresight device may be needed in addition to source, links and sinks. Or link is extended in some way to connect to non-coresight sources/sinks. Let me know if you have any thoughts. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CoreSight framework and drivers
On 12/20/2012 01:51 PM, Pratik Patel wrote: On Thu, Dec 20, 2012 at 11:46:13AM -0600, Jon Hunter wrote: On 12/19/2012 03:24 PM, Pratik Patel wrote: [snip] Currently we use the CoreSight virtual bus to conveniently list sysfs configuration attributes for all the registered CoreSight devices. For eg: /sys/bus/coresight/devices/coresight-etm0/attribute /sys/bus/coresight/devices/coresight-etm1/attribute /sys/bus/coresight/devices/coresight-stm/attribute /sys/bus/coresight/devices/coresight-tmc-etf/attribute ... ... Some of the attributes are based on device type (i.e. source, link or sink) so they will exist for all devices of that type while some are device specific. Maybe I am misunderstanding the question but are you suggesting to register CoreSight devices to the AMBA bus instead of the CoreSight core layer code? Yes exactly. Will Deacon mentioned earlier that AMBA framework can be changed to accomodate devices with a different class but I am more concerned with losing some of the stuff that the core layer code does (eg. coresight_register, coresight_enable, coresight_disable in coresight.c) if we register CoreSight drivers to the AMBA bus without letting the core layer know about the device connections. I may be missing something, but couldn't you keep all the register/enable/disable stuff but just register the device with the amba bus? Obviously some changes would need to be made. Ok, so are you referring to making CoreSight devices register with AMBA bus instead of platform bus keeping everything else intact? Yes exactly. However, please note I am not saying that we should do this, and I asking what direction does the community want us to take here? Platform bus or AMBA bus? Personally, I don't have strong feeling either way, but we have ETM/ETB drivers using AMBA today and so I am hoping we can come to agreement on this going forward. Russell, Will, what are your thoughts? Otherwise, looking at the code, I like what you have implemented. I still need to look closer, but I am struggling to figure out how a coresight device such as the cross-trigger-interface fits with this model. This model appears to be geared towards coresight devices used for traces purposes and are either source, links or sinks. The cross-trigger-interface is not a source or a sink. However, although you it could be considered as a link (routing events), it is not really, as it may not link to other coresight sinks/source. In my case, I have PMU-IRQ -- CTI -- GIC. So a non-coresight source and sink. In away the CTI looks more like a 2nd-level interrupt controller than anything else. Hence, another type of coresight device may be needed in addition to source, links and sinks. Or link is extended in some way to connect to non-coresight sources/sinks. Let me know if you have any thoughts. I had left the None type for miscellaneous stuff but I agree it should be a separate type - maybe debug. Having said that I like the CTI driver you have uploaded. Need to look at it in more detail. Since CTI connections can vary between chip to chip, we need a generic way to deal with it. Yes if you have any ideas let me know. As Will had mentioned it would be good to have a common function table all these devices could use too. I will take a closer look at what you have. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: CoreSight framework and drivers
On 12/21/2012 04:18 PM, Pratik Patel wrote: On Thu, Dec 20, 2012 at 04:54:38PM -0600, Jon Hunter wrote: On 12/20/2012 01:51 PM, Pratik Patel wrote: On Thu, Dec 20, 2012 at 11:46:13AM -0600, Jon Hunter wrote: On 12/19/2012 03:24 PM, Pratik Patel wrote: [snip] Currently we use the CoreSight virtual bus to conveniently list sysfs configuration attributes for all the registered CoreSight devices. For eg: /sys/bus/coresight/devices/coresight-etm0/attribute /sys/bus/coresight/devices/coresight-etm1/attribute /sys/bus/coresight/devices/coresight-stm/attribute /sys/bus/coresight/devices/coresight-tmc-etf/attribute ... ... Some of the attributes are based on device type (i.e. source, link or sink) so they will exist for all devices of that type while some are device specific. Maybe I am misunderstanding the question but are you suggesting to register CoreSight devices to the AMBA bus instead of the CoreSight core layer code? Yes exactly. Will Deacon mentioned earlier that AMBA framework can be changed to accomodate devices with a different class but I am more concerned with losing some of the stuff that the core layer code does (eg. coresight_register, coresight_enable, coresight_disable in coresight.c) if we register CoreSight drivers to the AMBA bus without letting the core layer know about the device connections. I may be missing something, but couldn't you keep all the register/enable/disable stuff but just register the device with the amba bus? Obviously some changes would need to be made. Ok, so are you referring to making CoreSight devices register with AMBA bus instead of platform bus keeping everything else intact? Yes exactly. However, please note I am not saying that we should do this, and I asking what direction does the community want us to take here? Platform bus or AMBA bus? Personally, I don't have strong feeling either way, but we have ETM/ETB drivers using AMBA today and so I am hoping we can come to agreement on this going forward. Russell, Will, what are your thoughts? Otherwise, looking at the code, I like what you have implemented. I still need to look closer, but I am struggling to figure out how a coresight device such as the cross-trigger-interface fits with this model. This model appears to be geared towards coresight devices used for traces purposes and are either source, links or sinks. The cross-trigger-interface is not a source or a sink. However, although you it could be considered as a link (routing events), it is not really, as it may not link to other coresight sinks/source. In my case, I have PMU-IRQ -- CTI -- GIC. So a non-coresight source and sink. In away the CTI looks more like a 2nd-level interrupt controller than anything else. Hence, another type of coresight device may be needed in addition to source, links and sinks. Or link is extended in some way to connect to non-coresight sources/sinks. Let me know if you have any thoughts. I had left the None type for miscellaneous stuff but I agree it should be a separate type - maybe debug. Having said that I like the CTI driver you have uploaded. Need to look at it in more detail. Since CTI connections can vary between chip to chip, we need a generic way to deal with it. Yes if you have any ideas let me know. As Will had mentioned it would be good to have a common function table all these devices could use too. I will take a closer look at what you have. I had started on a CTI driver much in line with your current implementation but your approach looks good to me. Looking at the code though, I couldn't find a way to perform a mapping between two different CTIs. Reason I ask is because we have use cases where we need to map one CTIs input to another CTIs output on the same channel. The code is largely based upon the existing cti helpers, which just had a cti_map_trigger() function. The use-case you described is not supported by the current helpers and so also not supported in my initial implementation (although we should add this). Hence, it would be probably best to split the cti_map_trigger() into two functions say, cti_map_triggerin() and cti_map_triggerout(), to map a trigger input/output to a channel. Do you intend to support different entities within kernel to map different trig_in or trig_out on the same CTI? If so, it might probably require reference counting for the enable/disable. I need to think about this some more, as this does make it a little more complex. Right now only one entity can use a cti module at a time. What user interface do you plan to provide for the CTI? Maybe something consistent with other CoreSight components in sysfs to allow users to enable, disable, map and unmap ??? Yes that's possible. Please let me know your thoughts. CTIs can easily be made part of the CoreSight core layer using a different type but apart from being part of the CoreSight list containing all the trace and CTI
Re: [PATCH] OMAP: add pwm driver using dmtimers.
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
+ depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX)+= pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS)+= pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA)+= pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG)+= pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) pwm-omap: fmt
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
+#define DM_TIMER_LOAD_MIN0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#define pwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct omap_chip *omap = to_omap_chip(chip);
+ int status = 0;
+
+ /* Enable the counter--always--before attempting to write its
+ * registers and then set the timer to its minimum load value to
+ * ensure we get an overflow event right away once we start it.
+ */
+
+ omap_dm_timer_enable(omap-dm_timer);
+ omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+ omap_dm_timer_start(omap-dm_timer);
+
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 05:31 AM, Thierry Reding wrote:
On Wed, Dec 12, 2012 at 07:24:30PM +1100, NeilBrown wrote:
[snip]
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+struct omap_chip *omap = to_omap_chip(chip);
+int status = 0;
+
+/* Enable the counter--always--before attempting to write its
+ * registers and then set the timer to its minimum load value to
+ * ensure we get an overflow event right away once we start it.
+ */
Block comments should be in the following format:
/*
* foo...
* bar...
*/
+
+omap_dm_timer_enable(omap-dm_timer);
+omap_dm_timer_write_counter(omap-dm_timer, DM_TIMER_LOAD_MIN);
+omap_dm_timer_start(omap-dm_timer);
+omap_dm_timer_disable(omap-dm_timer);
So omap_dm_timer_disable() doesn't actually stop the timer? It just
disables the access to the registers?
I thought this looked odd too ;-)
So what is going on here is that omap_dm_timer_start() calls
omap_dm_timer_enable() but does not call omap_dm_timer_disable(). So the
last disable really just complements the first enable (ie. decrements
the use count), but the timer will not actually be disabled, because the
start has called an extra enable.
These four function calls can be replaced by one call to
omap_dm_timer_set_load_start() and I think that will be much clearer and
concise.
In general, it should not be necessary to call these
omap_dm_timer_enable/disable APIs directly. I am not sure what the
history is or if there is a use-case that really requires this. So in
the future may be I should make them static so they cannot be used
directly :-)
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 09:06 PM, NeilBrown wrote:
[Thierry: question for you near the end - thanks]
On Wed, 12 Dec 2012 10:08:28 -0600 Jon Hunter jon-hun...@ti.com wrote:
Hi Neil,
On 12/12/2012 02:24 AM, NeilBrown wrote:
This patch is based on an earlier patch by Grant Erickson
which provided pwm devices using the 'legacy' interface.
This driver instead uses the new framework interface.
Cc: Grant Erickson maratho...@gmail.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ed81720..7df573a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -85,6 +85,15 @@ config PWM_MXS
To compile this driver as a module, choose M here: the module
will be called pwm-mxs.
+config PWM_OMAP
+ tristate OMAP pwm support
+ depends on ARCH_OMAP
We should probably have depends on or selects OMAP_DM_TIMER here too.
Sounds sensible - fixed.
+ help
+ Generic PWM framework driver for OMAP
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-omap
+
config PWM_PUV3
tristate PKUnity NetBook-0916 PWM support
depends on ARCH_PUV3
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index acfe482..f5d200d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PWM_IMX) += pwm-imx.o
obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_MXS) += pwm-mxs.o
+obj-$(CONFIG_PWM_OMAP) += pwm-omap.o
obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o
obj-$(CONFIG_PWM_PXA) += pwm-pxa.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
diff --git a/drivers/pwm/pwm-omap.c b/drivers/pwm/pwm-omap.c
new file mode 100644
index 000..e3dbce3
--- /dev/null
+++ b/drivers/pwm/pwm-omap.c
@@ -0,0 +1,318 @@
+/*
+ *Copyright (c) 2012 NeilBrown ne...@suse.de
+ *Heavily based on earlier code which is:
+ *Copyright (c) 2010 Grant Erickson maratho...@gmail.com
+ *
+ *Also based on pwm-samsung.c
+ *
+ *This program is free software; you can redistribute it and/or
+ *modify it under the terms of the GNU General Public License
+ *version 2 as published by the Free Software Foundation.
+ *
+ *Description:
+ * This file is the core OMAP2/3 support for the generic, Linux
I would drop the OMAP2/3 and just say OMAP here. Potentially this should
work for OMAP1-5.
Done.
+ * PWM driver / controller, using the OMAP's dual-mode timers.
+ *
+ *The 'id' number for the device encodes the number of the dm timer
+ *to use, and the polarity of the output.
+ *lsb is '1' of active-high, and '0' for active low
+ *remaining bit a timer number and need to be shifted down before use.
+ */
+
+#define pr_fmt(fmt) pwm-omap: fmt
+
+#include linux/export.h
+#include linux/kernel.h
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/err.h
+#include linux/clk.h
+#include linux/io.h
+#include linux/pwm.h
+#include linux/module.h
+
+#include plat/dmtimer.h
This is going to be a problem for the single zImage work, because we
cannot include any plat headers in driver code any more. Therefore,
although it is not ideal, one way to handle this is pass function
pointers to the various dmtimer APIs that are needed via the platform
data. Painful I know ...
But that doesn't work with devicetree does it?
Ugh, you are right! This is becoming an increasing problem.
Can't we move the dmtimer.h file to include/linux/omap-dmtimer.h or something?
I can ask Tony if he thinks we could do that.
It only included other things from include/linux, so it should be safe.
+#define DM_TIMER_LOAD_MIN 0xFFFE
+
+struct omap_chip {
+ struct platform_device *pdev;
+
+ struct omap_dm_timer*dm_timer;
+ unsigned intpolarity;
+ const char *label;
+
+ unsigned intduty_ns, period_ns;
+ struct pwm_chip chip;
+};
+
+#define to_omap_chip(chip) container_of(chip, struct omap_chip, chip)
+
+#definepwm_dbg(_pwm, msg...) dev_dbg((_pwm)-pdev-dev, msg)
+
+/**
+ * pwm_calc_value - determines the counter value for a clock rate and
period.
+ * @clk_rate: The clock rate, in Hz, of the PWM's clock source to compute
the
+ *counter value for.
+ * @ns: The period, in nanoseconds, to computer the counter value for.
+ *
+ * Returns the PWM counter value for the specified clock rate and period.
+ */
+static inline int pwm_calc_value(unsigned long clk_rate, int ns)
+{
+ const unsigned long nanoseconds_per_second = 10;
+ int cycles;
+ __u64 c;
+
+ c = (__u64)clk_rate * ns;
+ do_div(c, nanoseconds_per_second);
+ cycles = c;
+
+ return DM_TIMER_LOAD_MIN - cycles;
+}
+
+static int omap_pwm_enable(struct pwm_chip *chip, struct pwm_device
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 12/12/2012 10:33 PM, NeilBrown wrote: On Thu, 13 Dec 2012 14:06:35 +1100 NeilBrown ne...@suse.de wrote: + omap_dm_timer_enable(omap-dm_timer); Do you need to call omap_dm_timer_enable here? _set_load and _set_match will enable the timer. So this should not be necessary. True. That is what you get for copying someone else's code and not understanding it fully. However omap_dm_timer_write_counter *doesn't* enable the timer, and explicitly checks that it is already runtime-enabled. Does that mean I don't need to call omap_dm_timer_write_counter here? Or does it mean that I do need the enable/disable pair? Typically, omap_dm_timer_write_counter() is used to update the counter value while the counter is running and hence is enabled. Looking at the code, some more I now see what they are trying to do. It seems that they are trying to force an overflow to occur as soon as they enable the timer. This will cause the timer to load the count value from the timer load register into the timer counter register. So that does make sense to me. However, this should not be necessary as omap_dm_timer_set_load should do this for you. Therefore, I think that you could accomplish the same thing by doing ... omap_pwm_config -- omap_dm_timer_set_load() -- omap_dm_timer_set_match() -- omap_dm_timer_set_pwm() omap_pwm_enable -- omap_dm_timer_start() If we call _set_load in config then we don't need to call _load_start in the enable, we can just call _start. Can you try this and see if this is working ok? Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Remove inline from clock framework function definitions to build the kernel with GCC 4.7
On 11/15/2012 11:07 AM, Igor Mazanov wrote: Remove inline from clock framework function definitions to build the kernel with GCC 4.7 Adding Mike to the party ... May be good to add some details about the exact problem seen. I am seeing the same problem today with GCC 4.7 and Tony's master branch. For a bit of background it seems that for 4.7 not having the body of the inlined function available in the header is causing this error. Another example here [1]. The actual compiler error seen for OMAP is ... In file included from arch/arm/mach-omap2/clockdomain.c:25:0: arch/arm/mach-omap2/clockdomain.c: In function ‘clkdm_clk_disable’: include/linux/clk-provider.h:338:12: error: inlining failed in call to always_inline ‘__clk_get_enable_count’: function body not available arch/arm/mach-omap2/clockdomain.c:1001:28: error: called from here make[1]: *** [arch/arm/mach-omap2/clockdomain.o] Error 1 make: *** [arch/arm/mach-omap2] Error 2 Signed-off-by: Igor Mazanov i.maza...@gmail.com --- include/linux/clk-provider.h |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index c127315..f9f5e9e 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -335,8 +335,8 @@ const char *__clk_get_name(struct clk *clk); struct clk_hw *__clk_get_hw(struct clk *clk); u8 __clk_get_num_parents(struct clk *clk); struct clk *__clk_get_parent(struct clk *clk); -inline int __clk_get_enable_count(struct clk *clk); -inline int __clk_get_prepare_count(struct clk *clk); +int __clk_get_enable_count(struct clk *clk); +int __clk_get_prepare_count(struct clk *clk); unsigned long __clk_get_rate(struct clk *clk); unsigned long __clk_get_flags(struct clk *clk); int __clk_is_enabled(struct clk *clk); Do we also need to remove the inline from the functions declared in drivers/clk/clk.c too? Cheers Jon [1] https://bugs.launchpad.net/linaro-android/+bug/983496 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] ARM: ux500: Describe UART platform registering issues more accurately
On 11/16/2012 03:36 AM, Arnd Bergmann wrote: On Thursday 15 November 2012, Lee Jones wrote: On Thu, 15 Nov 2012, Arnd Bergmann wrote: On Thursday 15 November 2012, Lee Jones wrote: UARTs no longer require call-back information, since the reset call-back was removed in 43b5f0d69291374f602ad8e1817f329573a59010. The only AUXDATA dependencies remaining for UARTs are DMA settings. Signed-off-by: Lee Jones lee.jo...@linaro.org Acked-by: Arnd Bergmann a...@arndb.de What is the state of the DMA binding now? We originally wanted to merge it for 3.7, but that didn't work out, so I hope we can get it done for 3.8. It's dead. The conversation has been stale for weeks. Perhaps you'd like to go and poke it? :) Vinod, is there anything you are waiting for still? Should Jon resend his latest patches to make sure we get them merged this time? I have multiple people that want to send me patches for 3.8 based on that work, so we are running out of time now. Vinod responded today saying it will be in for v3.8 [1]. Jon [1] http://article.gmane.org/gmane.linux.ports.arm.omap/89502 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] OMAP: add pwm driver using dmtimers.
On 01/06/2013 03:12 PM, NeilBrown wrote:
[snip]
I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).
Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(
Some minor comments below ...
NeilBrown
From: NeilBrown ne...@suse.de
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.
Nit, subject should formatted ARM: OMAP: blah blah blah
Also, may be worth calling this fix context-loss as this is really
fixing something that is broken.
The context loss handling in dmtimer appears to assume that
omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
omap_dm_timer_stop()
bracket all interactions. Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.
So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).
Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
pm_runtime_get_sync(timer-pdev-dev);
+
+ if (!(timer-capability OMAP_TIMER_ALWON)) {
+ int loss_count =
+ omap_pm_get_dev_context_loss_count(timer-pdev-dev);
+ if (loss_count != timer-ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer-ctx_loss_count = loss_count;
+ }
+ }
}
Can you rebase on v3.8-rc2? We no longer call
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
void omap_dm_timer_enable(struct omap_dm_timer *timer)
{
+ int c;
+
pm_runtime_get_sync(timer-pdev-dev);
+
+ if (!(timer-capability OMAP_TIMER_ALWON)) {
+ if (timer-get_context_loss_count) {
+ c = timer-get_context_loss_count(timer-pdev-dev);
+ if (c != timer-ctx_loss_count) {
+ omap_timer_restore_context(timer);
+ timer-ctx_loss_count = c;
+ }
+ }
+ }
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] omap: DT node Timer iteration fix
Hi Pantelis,
On 01/08/2013 07:31 AM, Pantelis Antoniou wrote:
The iterator correctly handles of_node_put() calls.
Remove it before continue'ing the loop.
Without this patch you get:
Thanks for the fix!
May be worth mentioning that this will only be seen with
CONFIG_OF_DYNAMIC (and explains why I did not catch this one!).
ERROR: Bad of_node_put() on /ocp/timer@44e31000!
[c001329c] (unwind_backtrace+0x0/0xe0) from [c03dd8f0]
(of_node_release+0x2c/0xa0)!
[c03dd8f0] (of_node_release+0x2c/0xa0) from [c03ddea0]
(of_find_matching_node_and_match+0x78/0x90)!
[c03ddea0] (of_find_matching_node_and_match+0x78/0x90) from [c06d349c]
(omap_get_timer_dt+0x78/0x90)!
[c06d349c] (omap_get_timer_dt+0x78/0x90) from [c06d3664]
(omap_dm_timer_init_one.clone.2+0x34/0x2bc)!
[c06d3664] (omap_dm_timer_init_one.clone.2+0x34/0x2bc) from [c06d3a2c]
(omap2_gptimer_clocksource_init.clone.4+0x24/0xa8)!
[c06d3a2c] (omap2_gptimer_clocksource_init.clone.4+0x24/0xa8) from
[c06cca58] (time_init+0x20/0x30)!
[c06cca58] (time_init+0x20/0x30) from [c06c9690]
(start_kernel+0x1a8/0x2fc)!
Signed-off-by: Pantelis Antoniou pa...@antoniou-consulting.com
---
arch/arm/mach-omap2/timer.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 691aa67..b8ad6e6 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -165,15 +165,11 @@ static struct device_node * __init
omap_get_timer_dt(struct of_device_id *match,
struct device_node *np;
for_each_matching_node(np, match) {
- if (!of_device_is_available(np)) {
- of_node_put(np);
+ if (!of_device_is_available(np))
continue;
- }
- if (property !of_get_property(np, property, NULL)) {
- of_node_put(np);
+ if (property !of_get_property(np, property, NULL))
continue;
- }
of_add_property(np, device_disabled);
return np;
Otherwise ...
Acked-by: Jon Hunter jon-hun...@ti.com
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 11/14] Documentation: dt: binding: omap: am43x timer
On 28/05/13 23:05, Stephen Warren wrote: On 05/28/2013 03:25 PM, Jon Hunter wrote: On 27/05/13 15:37, Afzal Mohammed wrote: AM43x timer bindings. Signed-off-by: Afzal Mohammed af...@ti.com --- Documentation/devicetree/bindings/arm/omap/timer.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt index d02e27c..70cb398 100644 --- a/Documentation/devicetree/bindings/arm/omap/timer.txt +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt @@ -14,6 +14,8 @@ Required properties: ti,omap5430-timer (applicable to OMAP543x devices) ti,am335x-timer (applicable to AM335x devices) ti,am335x-timer-1ms (applicable to AM335x devices) + ti,am4372-timer-1ms, ti,am335x-timer-1ms for AM43x 1ms timer + ti,am4372-timer, ti,am335x-timer for AM43x timers other than 1ms one - reg: Contains timer register address range (base address and length). If you are adding more compatibility strings, then this implies that the AM43x timers are not 100% compatible with any other device listed (such as am335x or any omap device). That's fine but you should state that in the changelog. If the AM43x timer registers are 100% compatible with existing devices you should not add these. I'm not sure that's true; .dts files should always include a compatible value that describes the most specific model of the HW, plus any baseline compatible value that the HW is compatible with. This allows any required quirks/fixes/... to be applied for the specific HW model later even if nobody knows right now they'll be needed. Hence, defining new compatible values doesn't necessarily mean incompatible HW. Yes that does seem to agree with what Grant had told me in the past [1]. So this is ok then. However, I still think that the changelog needs to be vastly improved and more explicit about the changes. Cheers Jon [1] http://thread.gmane.org/gmane.linux.ports.arm.omap/90551/focus=26447 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: of: Remove check on always true condition
On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
Both of_dma_nbcells field of the of_dma_controller and the args_count field of
the dma_spec are initialized by parsing the #dma-cells attribute of their
device
tree node. So if the device tree nodes of a DMA controller and the dma_spec
match this means that of_dma_nbcells and args_count will also match. So the
second test in the of_dma_find_controller loop is redundant because given the
first test yields true the second test will also yield true. So we can safely
remove the test whether of_dma_nbcells matches args_count. Since this was the
last user of the of_dma_nbcells field we can remove it altogether.
This assumes that someone has correctly added the dma information to the
dma slave binding. I could see systems where different dma controllers
have different of_dma_nbcells and so someone could put the enter wrong
number of cells for a dma slave binding. Its really to catch user error.
Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
drivers/dma/of-dma.c | 14 +-
include/linux/of_dma.h | 1 -
2 files changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c
index 268cc8a..75334bd 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -35,8 +35,7 @@ static struct of_dma *of_dma_find_controller(struct
of_phandle_args *dma_spec)
struct of_dma *ofdma;
list_for_each_entry(ofdma, of_dma_list, of_dma_controllers)
- if ((ofdma-of_node == dma_spec-np)
- (ofdma-of_dma_nbcells == dma_spec-args_count))
+ if (ofdma-of_node == dma_spec-np)
return ofdma;
Other device-tree functions perform similar tests to this such as ...
static int of_gpiochip_find_and_xlate(struct gpio_chip *gc, void *data)
{
struct gg_data *gg_data = data;
int ret;
if ((gc-of_node != gg_data-gpiospec.np) ||
(gc-of_gpio_n_cells != gg_data-gpiospec.args_count) ||
(!gc-of_xlate))
return false;
...
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] dma: of: Remove check on always true condition
On 04/22/2013 04:00 PM, Lars-Peter Clausen wrote:
On 04/22/2013 10:52 PM, Jon Hunter wrote:
On 04/22/2013 03:33 AM, Lars-Peter Clausen wrote:
Both of_dma_nbcells field of the of_dma_controller and the args_count field
of
the dma_spec are initialized by parsing the #dma-cells attribute of their
device
tree node. So if the device tree nodes of a DMA controller and the dma_spec
match this means that of_dma_nbcells and args_count will also match. So the
second test in the of_dma_find_controller loop is redundant because given
the
first test yields true the second test will also yield true. So we can
safely
remove the test whether of_dma_nbcells matches args_count. Since this was
the
last user of the of_dma_nbcells field we can remove it altogether.
This assumes that someone has correctly added the dma information to the
dma slave binding. I could see systems where different dma controllers
have different of_dma_nbcells and so someone could put the enter wrong
number of cells for a dma slave binding. Its really to catch user error.
No, this assumes nothing. The condition will _always_ be true.
dma_spec-args_count is initialized by parsing the #dma-cells attribute of
dma_sepc-np. of_dma-of_dma_nbcells is initialized by parsing the #dma-cells
attribute of of_dma-of_node. If ofdma-of_node equals dma_spec-np then
dma_spec-args_count will also equal of_dma-of_dma_nbcells.
Thanks for the clarification. I should have looked more closely at
of_parse_phandle_with_args().
Yes I agree it will always be true if count is less than or equal to
MAX_PHANDLE_ARGS (which defaults to 8). It is very unlikely that someone
would use more than 8 and I guess it does warn on this condition.
if (out_args) {
int i;
if (WARN_ON(count MAX_PHANDLE_ARGS))
count = MAX_PHANDLE_ARGS;
out_args-np = node;
out_args-args_count = count;
for (i = 0; i count; i++)
out_args-args[i] = be32_to_cpup(list++);
}
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: OMAP2+: timer: initialize before using oh_name
On 28/05/13 07:24, Afzal Mohammed wrote:
of_property_read_string_index(...,oh_name) in omap_dm_timer_init_one
does not alter the value of 'oh_name' even if the relevant function
fails and as 'oh_name' in stack may have a non-zero value, it would
be misunderstood by timer code that DT has specified ti,hwmod
property for timer. 'oh_name' in this scenario would be a junk value,
this would result in module not being enabled by hwmod API's for
timer, and in turn crash.
Signed-off-by: Afzal Mohammed af...@ti.com
---
arch/arm/mach-omap2/timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index f8b23b8..8e0c390 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -220,7 +220,7 @@ static int __init omap_dm_timer_init_one(struct
omap_dm_timer *timer,
int posted)
{
char name[10]; /* 10 = sizeof(gptXX_Xck0) */
- const char *oh_name;
+ const char *oh_name = NULL;
struct device_node *np;
struct omap_hwmod *oh;
struct resource irq, mem;
Thanks!
Acked-by: Jon Hunter jgchun...@gmail.com
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 12/14] Documentation: dt: binding: omap: am43x counter
On 27/05/13 15:37, Afzal Mohammed wrote: AM43x 32K counter binding. Signed-off-by: Afzal Mohammed af...@ti.com --- Documentation/devicetree/bindings/arm/omap/counter.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/arm/omap/counter.txt b/Documentation/devicetree/bindings/arm/omap/counter.txt index 5bd8aa0..9c48dca 100644 --- a/Documentation/devicetree/bindings/arm/omap/counter.txt +++ b/Documentation/devicetree/bindings/arm/omap/counter.txt @@ -2,6 +2,7 @@ OMAP Counter-32K bindings Required properties: - compatible:Must be ti,omap-counter32k for OMAP controllers + ti,am4372-counter32k,ti,omap-counter32k for AM43x counter - reg: Contains timer register address range (base address and length) - ti,hwmods: Name of the hwmod associated to the counter, which is typically counter_32k Changelog should state why this is needed. Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 11/14] Documentation: dt: binding: omap: am43x timer
On 27/05/13 15:37, Afzal Mohammed wrote: AM43x timer bindings. Signed-off-by: Afzal Mohammed af...@ti.com --- Documentation/devicetree/bindings/arm/omap/timer.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt index d02e27c..70cb398 100644 --- a/Documentation/devicetree/bindings/arm/omap/timer.txt +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt @@ -14,6 +14,8 @@ Required properties: ti,omap5430-timer (applicable to OMAP543x devices) ti,am335x-timer (applicable to AM335x devices) ti,am335x-timer-1ms (applicable to AM335x devices) + ti,am4372-timer-1ms, ti,am335x-timer-1ms for AM43x 1ms timer + ti,am4372-timer, ti,am335x-timer for AM43x timers other than 1ms one - reg: Contains timer register address range (base address and length). If you are adding more compatibility strings, then this implies that the AM43x timers are not 100% compatible with any other device listed (such as am335x or any omap device). That's fine but you should state that in the changelog. If the AM43x timer registers are 100% compatible with existing devices you should not add these. Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: OMAP1: PM: fix some build warnings on 1510-only Kconfigs
Hi Paul,
On 02/11/2015 02:25 AM, Paul Walmsley wrote:
Hi John,
thanks for the review,
On Tue, 10 Feb 2015, Jon Hunter wrote:
[snip]
Subject: [PATCH] ARM: OMAP1: PM: fix some build warnings on 1510-only Kconfigs
Building an OMAP1510-only Kconfig generates the following warnings:
arch/arm/mach-omap1/pm.c: In function ‘omap1_pm_idle’:
arch/arm/mach-omap1/pm.c:123:2: warning: #warning Enable 32kHz OS timer in
order to allow sleep states in idle [-Wcpp]
#warning Enable 32kHz OS timer in order to allow sleep states in idle
^
arch/arm/mach-omap1/pm.c: At top level:
arch/arm/mach-omap1/pm.c:76:23: warning: ‘enable_dyn_sleep’ defined but not
used [-Wunused-variable]
static unsigned short enable_dyn_sleep = 0;
^
These are not so easy to fix in an obviously correct fashion, since I
don't have these devices up and running in my testbed. So, use
arch/arm/plat-omap/Kconfig and the existing pm.c source as a guide,
and posit that deep power saving states are only supported on OMAP16xx
chips with kernels built with both CONFIG_OMAP_DM_TIMER=y and
CONFIG_OMAP_32K_TIMER=y.
While here, clean up a few printk()s and unnecessary #ifdefs.
This second version of the patch incorporates several suggestions from
Jon Hunter jgchun...@gmail.com.
Signed-off-by: Paul Walmsley p...@pwsan.com
Cc: Jon Hunter jonath...@nvidia.com
Cc: Aaro Koskinen aaro.koski...@iki.fi
Cc: Tuukka Tikkanen tuukka.tikka...@linaro.org
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Tony Lindgren t...@atomide.com
Cc: Russell King li...@arm.linux.org.uk
Cc: linux-o...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Jon Hunter jgchun...@gmail.com
---
arch/arm/mach-omap1/pm.c | 51
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c
index 34b4c0044961..dd94567c3628 100644
--- a/arch/arm/mach-omap1/pm.c
+++ b/arch/arm/mach-omap1/pm.c
@@ -71,13 +71,7 @@ static unsigned int
mpui7xx_sleep_save[MPUI7XX_SLEEP_SAVE_SIZE];
static unsigned int mpui1510_sleep_save[MPUI1510_SLEEP_SAVE_SIZE];
static unsigned int mpui1610_sleep_save[MPUI1610_SLEEP_SAVE_SIZE];
-#ifndef CONFIG_OMAP_32K_TIMER
-
-static unsigned short enable_dyn_sleep = 0;
-
-#else
-
-static unsigned short enable_dyn_sleep = 1;
+static unsigned short enable_dyn_sleep;
static ssize_t idle_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
@@ -90,8 +84,9 @@ static ssize_t idle_store(struct kobject *kobj, struct
kobj_attribute *attr,
{
unsigned short value;
if (sscanf(buf, %hu, value) != 1 ||
- (value != 0 value != 1)) {
- printk(KERN_ERR idle_sleep_store: Invalid value\n);
+ (value != 0 value != 1) ||
+ (value != 0 !IS_ENABLED(CONFIG_OMAP_32K_TIMER))) {
+ pr_err(idle_sleep_store: Invalid value\n);
return -EINVAL;
}
enable_dyn_sleep = value;
@@ -101,7 +96,6 @@ static ssize_t idle_store(struct kobject *kobj, struct
kobj_attribute *attr,
static struct kobj_attribute sleep_while_idle_attr =
__ATTR(sleep_while_idle, 0644, idle_show, idle_store);
-#endif
static void (*omap_sram_suspend)(unsigned long r0, unsigned long r1) = NULL;
@@ -115,16 +109,11 @@ void omap1_pm_idle(void)
{
extern __u32 arm_idlect1_mask;
__u32 use_idlect1 = arm_idlect1_mask;
- int do_sleep = 0;
local_fiq_disable();
#if defined(CONFIG_OMAP_MPU_TIMER) !defined(CONFIG_OMAP_DM_TIMER)
-#warning Enable 32kHz OS timer in order to allow sleep states in idle
Thinking about this some more, I don't understand the dependency on the
DM_TIMER here. For an omap1 device, regardless of whether the DM_TIMERs
are enable or not, the device should be able to enter low power if the
32K is enabled. Hence, shouldn't this have been !(CONFIG_OMAP_32K_TIMER)
above?
Furthermore, you will get the above warning on a omap16xx only build if
you disable DM_TIMERs and keep MPU_TIMER enabled, which should be a
valid thing to do.
Tony, I see you added the DM_TIMER dependency in commit
be26a008414414c69a4ae9fe9877401c3ba62c5a. I understand your motivation,
but why not just use !(CONFIG_OMAP_32K_TIMER) here? Bit 9 of the idlect1
is only for the TIMCK clock that is used for the MPU timers and not the
DM_TIMERs.
-#if defined(CONFIG_OMAP_MPU_TIMER) !defined(CONFIG_OMAP_DM_TIMER)
-#warning Enable 32kHz OS timer in order to allow sleep states in idle
+#if !defined(CONFIG_OMAP_32K_TIMER)
use_idlect1 = use_idlect1 ~(1 9);
-#else
- if (enable_dyn_sleep)
- do_sleep = 1;
#endif
use_idlect1 = use_idlect1 ~(1 9);
-#else
- if (enable_dyn_sleep)
- do_sleep = 1;
#endif
#ifdef CONFIG_OMAP_DM_TIMER
@@ -134,10 +123,12 @@ void omap1_pm_idle(void
Re: [PATCH] ARM: OMAP1: PM: fix some build warnings on 1510-only Kconfigs
On 02/11/2015 09:14 PM, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [150211 13:03]: On Wed, 11 Feb 2015, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [150210 18:28]: On Tue, 10 Feb 2015, Jon Hunter wrote: On 07/02/2015 00:23, Paul Walmsley wrote: Unfortunately, there is not a single TRM for the omap5910 but individual documents for each chapter in the original TRM. Check out the OMAP5910 Dual-Core Processor Timer Reference Guide and possibly the OMAP5910 Dual-Core Processor Clock Generation and System Reset Management Reference Guide The omap15xx/5910 did have a 32k timer but as you can see it appears it was never supported by the kernel for this device (not sure why). I do recall that there is some errata regarding the 32k timer, if you look at the omap5910 errata document and search for 32k you should find it. OK thanks for the context. I probably am not going to investigate adding support for this timer on OMAP1510/5910 - am primarily trying to avoid causing a regression on the existing platforms. At least I've never seen the 32KiHz timer registers in any 15xx documentation. Jon are you sure you're not mixing up 5910 (15xx) and 5912 (16xx)? It's documented in the OMAP5910 Timer Reference Guide (SPRU682A) Section 3 32-kHz Timer, at the link Jon mentioned. Have not checked the errata that Jon mentioned though. Interesting. Looks like it's the same as on 16xx at 0xfffb9000. AFAIK that never worked on 15xx. Or maybe the issue was that 15xx is missing the constantly running 32KiHz counter making the timer unusable from PM point of view as the clockevent alone is not enough. Regarding the patch: I'd suggest keeping the compilation warning fixes (which was the original purpose of the patch) from anything that changes the logic too much. That way if there's an error in the patch that changes the logic and it needs to be reverted, it won't also revert the warning fixes. Makes sense to me. Yes that's fine with me as well, I don't wish to over complicate matters. I have a couple minor comments though and will respond to the latest patch rev. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: OMAP1: PM: fix some build warnings on 1510-only Kconfigs
Hi Paul,
On 07/02/2015 00:23, Paul Walmsley wrote:
Building an OMAP1510-only Kconfig generates the following warnings:
arch/arm/mach-omap1/pm.c: In function ‘omap1_pm_idle’:
arch/arm/mach-omap1/pm.c:123:2: warning: #warning Enable 32kHz OS timer
in order to allow sleep states in idle [-Wcpp]
#warning Enable 32kHz OS timer in order to allow sleep states in idle
^
arch/arm/mach-omap1/pm.c: At top level:
arch/arm/mach-omap1/pm.c:76:23: warning: ‘enable_dyn_sleep’ defined but
not used [-Wunused-variable]
static unsigned short enable_dyn_sleep = 0;
^
These are not so easy to fix in an obviously correct fashion, since I
don't have these devices up and running in my testbed. So, use
arch/arm/plat-omap/Kconfig and the existing pm.c source as a guide,
and posit that deep power saving states are only supported on OMAP16xx
chips with kernels built with both CONFIG_OMAP_DM_TIMER=y and
CONFIG_OMAP_32K_TIMER=y.
While here, clean up a few printk()s and unnecessary #ifdefs.
Signed-off-by: Paul Walmsley p...@pwsan.com
Cc: Jon Hunter jonath...@nvidia.com
Cc: Aaro Koskinen aaro.koski...@iki.fi
Cc: Tuukka Tikkanen tuukka.tikka...@linaro.org
Cc: Kevin Hilman khil...@deeprootsystems.com
Cc: Tony Lindgren t...@atomide.com
Cc: Russell King li...@arm.linux.org.uk
Cc: linux-o...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
Hi folks, if anyone out there is still experimenting with OMAP1 PM, or has
a copy of the OMAP1510 TRMs, could you please check this patch? I'm
unable to test it since I don't have any OMAP1 devices currently active
in the testbed. It at least compiles and deals with the build warnings:
You can find the omap5910 documents here [1]. The omap5910 is equivalent
to the omap1510. Unfortunately, there is not a single TRM for the
omap5910 but individual documents for each chapter in the original TRM.
Check out the OMAP5910 Dual-Core Processor Timer Reference Guide and
possibly the OMAP5910 Dual-Core Processor
Clock Generation and System Reset Management Reference Guide
The omap15xx/5910 did have a 32k timer but as you can see it appears it
was never supported by the kernel for this device (not sure why). I do
recall that there is some errata regarding the 32k timer, if you look at
the omap5910 errata document and search for 32k you should find it.
I no longer have access to omap15xx h/w to test. However, I do have
omap5912 if you want me to test.
http://www.pwsan.com/omap/testlogs/fix-omap-warnings-v3.21/20150206154619/
Non-critical; targeted for v3.20-rc1 or v3.21-rc1.
arch/arm/mach-omap1/pm.c | 42 +-
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/arch/arm/mach-omap1/pm.c b/arch/arm/mach-omap1/pm.c
index 34b4c0044961..d46d8a222fbb 100644
--- a/arch/arm/mach-omap1/pm.c
+++ b/arch/arm/mach-omap1/pm.c
@@ -71,13 +71,7 @@ static unsigned int
mpui7xx_sleep_save[MPUI7XX_SLEEP_SAVE_SIZE];
static unsigned int mpui1510_sleep_save[MPUI1510_SLEEP_SAVE_SIZE];
static unsigned int mpui1610_sleep_save[MPUI1610_SLEEP_SAVE_SIZE];
-#ifndef CONFIG_OMAP_32K_TIMER
-
-static unsigned short enable_dyn_sleep = 0;
-
-#else
-
-static unsigned short enable_dyn_sleep = 1;
+static unsigned short enable_dyn_sleep;
static ssize_t idle_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
@@ -90,8 +84,9 @@ static ssize_t idle_store(struct kobject *kobj, struct
kobj_attribute *attr,
{
unsigned short value;
if (sscanf(buf, %hu, value) != 1 ||
- (value != 0 value != 1)) {
- printk(KERN_ERR idle_sleep_store: Invalid value\n);
+ (value != 0 value != 1) ||
+ (value != 0 !IS_ENABLED(CONFIG_OMAP_32K_TIMER))) {
+ pr_err(idle_sleep_store: Invalid value\n);
return -EINVAL;
}
enable_dyn_sleep = value;
@@ -101,7 +96,6 @@ static ssize_t idle_store(struct kobject *kobj,
struct kobj_attribute *attr,
static struct kobj_attribute sleep_while_idle_attr =
__ATTR(sleep_while_idle, 0644, idle_show, idle_store);
-#endif
static void (*omap_sram_suspend)(unsigned long r0, unsigned long r1) =
NULL;
@@ -120,12 +114,11 @@ void omap1_pm_idle(void)
local_fiq_disable();
#if defined(CONFIG_OMAP_MPU_TIMER) !defined(CONFIG_OMAP_DM_TIMER)
-#warning Enable 32kHz OS timer in order to allow sleep states in idle
use_idlect1 = use_idlect1 ~(1 9);
-#else
+#endif
+
if (enable_dyn_sleep)
do_sleep = 1;
Do we still need this do_sleep variable now? Looking at the code, I
think we could use enable_dyn_sleep directly.
-#endif
#ifdef CONFIG_OMAP_DM_TIMER
use_idlect1 = omap_dm_timer_modify_idlect_mask(use_idlect1);
@@ -635,15 +628,24 @@ static const struct platform_suspend_ops
omap_pm_ops = {
static int __init omap_pm_init(void)
{
-
-#ifdef
Re: [PATCH] ARM: OMAP1: PM: fix some build warnings on 1510-only Kconfigs
On 02/12/2015 11:26 AM, Jon Hunter wrote: On 02/11/2015 09:14 PM, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [150211 13:03]: On Wed, 11 Feb 2015, Tony Lindgren wrote: * Paul Walmsley p...@pwsan.com [150210 18:28]: On Tue, 10 Feb 2015, Jon Hunter wrote: On 07/02/2015 00:23, Paul Walmsley wrote: Unfortunately, there is not a single TRM for the omap5910 but individual documents for each chapter in the original TRM. Check out the OMAP5910 Dual-Core Processor Timer Reference Guide and possibly the OMAP5910 Dual-Core Processor Clock Generation and System Reset Management Reference Guide The omap15xx/5910 did have a 32k timer but as you can see it appears it was never supported by the kernel for this device (not sure why). I do recall that there is some errata regarding the 32k timer, if you look at the omap5910 errata document and search for 32k you should find it. OK thanks for the context. I probably am not going to investigate adding support for this timer on OMAP1510/5910 - am primarily trying to avoid causing a regression on the existing platforms. At least I've never seen the 32KiHz timer registers in any 15xx documentation. Jon are you sure you're not mixing up 5910 (15xx) and 5912 (16xx)? It's documented in the OMAP5910 Timer Reference Guide (SPRU682A) Section 3 32-kHz Timer, at the link Jon mentioned. Have not checked the errata that Jon mentioned though. Interesting. Looks like it's the same as on 16xx at 0xfffb9000. AFAIK that never worked on 15xx. Or maybe the issue was that 15xx is missing the constantly running 32KiHz counter making the timer unusable from PM point of view as the clockevent alone is not enough. Regarding the patch: I'd suggest keeping the compilation warning fixes (which was the original purpose of the patch) from anything that changes the logic too much. That way if there's an error in the patch that changes the logic and it needs to be reverted, it won't also revert the warning fixes. Makes sense to me. Yes that's fine with me as well, I don't wish to over complicate matters. I have a couple minor comments though and will respond to the latest patch rev. Actually, nevermind the latest version is fine with me. Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] arm: am33xx: DT quirks for am33xx based beaglebone variants
On 02/19/2015 06:28 PM, Pantelis Antoniou wrote: Hi Tony, On Feb 19, 2015, at 20:16 , Tony Lindgren t...@atomide.com wrote: * Pantelis Antoniou pantelis.anton...@konsulko.com [150218 07:03]: Implement DT quirks for the am33xx beaglebone boards. --- /dev/null +++ b/arch/arm/mach-omap2/am33xx-dt-quirks.c ... + +/* + * The board IDs for am33xx board are in an I2C EEPROM + * We are very early in the boot process so we have to + * read the EEPROM directly without using the I2C layer. + * + * Note that we rely on the bootloader setting up the muxes + * (which is the case for u-boot). + */ + +/* I2C Status Register (OMAP_I2C_STAT): */ +#define OMAP_I2C_STAT_XDR (1 14) /* TX Buffer draining */ +#define OMAP_I2C_STAT_RDR (1 13) /* RX Buffer draining */ +#define OMAP_I2C_STAT_BB (1 12) /* Bus busy */ +#define OMAP_I2C_STAT_ROVR (1 11) /* Receive overrun */ +#define OMAP_I2C_STAT_XUDF (1 10) /* Transmit underflow */ +#define OMAP_I2C_STAT_AAS (1 9)/* Address as slave */ +#define OMAP_I2C_STAT_BF (1 8)/* Bus Free */ +#define OMAP_I2C_STAT_XRDY (1 4)/* Transmit data ready */ +#define OMAP_I2C_STAT_RRDY (1 3)/* Receive data ready */ +#define OMAP_I2C_STAT_ARDY (1 2)/* Register access ready */ +#define OMAP_I2C_STAT_NACK (1 1)/* No ack interrupt enable */ +#define OMAP_I2C_STAT_AL (1 0)/* Arbitration lost int ena */ ... Uhh I don't like the idea of duplicating the i2c-omap.c driver under arch/arm.. And in general we should initialize things later rather than earlier. What's stopping doing these quirk checks later on time with just a regular device driver, something like drivers/misc/bbone-quirks.c? We have no choice; we are way early in the boot process, right after the device tree unflattening step. Can you elaborate with an example of why not? Why can't the overlay happen at a later stage in the kernel boot as Tony suggests? One thought would be that ideally devices that are dependent on a particular board variant would be disabled in the base DT blob until you know what board you are. However, that assumes that they can be initialised at a later stage in the boot process and may be for some regulators or other devices this is not possible. I know you mentioned some time restrictions for some devices, but I still don't see why it could not happen later. Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] dmaengine: Fix dma_get_any_slave_channel() handling of private channels
The function dma_get_any_slave_channel() allocates private DMA channels
by calling the internal private_candidate() function. However, when
doing so, if a channel is successfully allocated, neither the
DMA_PRIVATE flag is set or the privatecnt variable is incremented for
the DMA controller. This will cause the following problems ...
1. A DMA controller initialised with the DMA_PRIVATE flag set (ie.
channels should always be private) will become public incorrectly
when a channel is allocated and then released. This is
because:
- A DMA controller initialised with DMA_PRIVATE set will have
a initial privatecnt of 1.
- The privatecnt is not incremented by dma_get_any_slave_channel().
- When the channel is released via dma_release_channel(), the
privatecnt is decremented and the DMA_PRIVATE flag is cleared
because the privatecnt value is 0.
2. For a DMA controller initialised with the DMA_PRIVATE flag set, if
more than one DMA channel is allocated successfully via
dma_get_any_slave_channel() and then one channel is released, the
following issues can occur:
i). All channels currently allocated will appear as public because
the DMA_PRIVATE will be cleared (as described in #1).
ii). Subsequent calls to dma_get_any_slave_channel() will fail even
if there are channels available. The reason this fails is that
the private_candidate() function (called by
dma_get_any_slave_channel()) will detect the DMA controller is
not private but has active channels and so cannot allocate any
private channels (see below code snippet).
/* devices with multiple channels need special handling as we need to
* ensure that all channels are either private or public.
*/
if (dev-chancnt 1 !dma_has_cap(DMA_PRIVATE, dev-cap_mask))
list_for_each_entry(chan, dev-channels, device_node) {
/* some channels are already publicly allocated */
if (chan-client_count)
return NULL;
}
3. For a DMA controller initialised with the DMA_PRIVATE flag unset,
if a private channel is allocated via dma_get_any_slave_channel(),
then the DMA controller will still appear as public because the
DMA_PRIVATE flag is not set and this will cause:
i). The allocated channel to appear as public
ii). Prevent any further private channels being allocated via
dma_get_any_slave_channel() (because private_candidate() will
fail in the same way as described in 2.ii above).
Fix this by incrementing the privatecnt in dma_get_any_slave_channel().
If dma_get_any_slave_channel() allocates a channel also ensure the
DMA_PRIVATE flag is set, in case it was not before. If the privatecnt
becomes 0 then the DMA_PRIVATE flag should be cleared.
Cc: Stephen Warren swar...@nvidia.com
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
This issue was found when attempting to open and close a serial
interface, that uses DMA, multiple times on a tegra device. When
opening the serial device a 2nd time after closing, the DMA channel
allocation would fail.
drivers/dma/dmaengine.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 0e035a8cf401..03b0e22b4a68 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -571,11 +571,16 @@ struct dma_chan *dma_get_any_slave_channel(struct
dma_device *device)
chan = private_candidate(mask, device, NULL, NULL);
if (chan) {
+ dma_cap_set(DMA_PRIVATE, device-cap_mask);
+ device-privatecnt++;
err = dma_chan_get(chan);
if (err) {
pr_debug(%s: failed to get %s: (%d)\n,
__func__, dma_chan_name(chan), err);
chan = NULL;
+
+ if (--device-privatecnt == 0)
+ dma_cap_clear(DMA_PRIVATE, device-cap_mask);
}
}
--
2.3.6
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] ARM: tegra: Correct which USB controller has the UTMI pad registers
On 04/03/2015 08:21 AM, Tomeu Vizoso wrote: It should be the first controller, not the second. The indexes of the usb resets were also wrong and have been fixed. The issue was caused by the changes in 308efde (ARM: tegra: Add resets has-utmi-pad-registers flag to all USB PHYs) being misapplied by git due to the patch context being insufficient. This broke USB after 6261b06 (regulator: Defer lookup of supply to regulator_get), because it changed the order in which the controllers were probed. The fix for this issue was suggested by Mikko Perttunen and Tuomas Tynkkynen. Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com Cc: Mikko Perttunen mikko.perttu...@kapsi.fi Cc: Tuomas Tynkkynen ttynkky...@nvidia.com --- Hi, hope I have gotten it right this time, but please do check :) FWIW, this works for me. Tested-by: Jon Hunter jonath...@nvidia.com Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 0/9] Tegra xHCI support
Hi Andrew, On 04/05/15 18:36, Andrew Bresticker wrote: This series adds support for xHCI on NVIDIA Tegra SoCs. This includes: - patches 1, 2, and 3: minor cleanups for mailbox framework and xHCI, - patches 4 and 5: adding an MFD driver for the XUSB cmoplex, - patches 6 and 7: adding a driver for the mailbox used to communicate with the xHCI controller's firmware, and - patches 8 and 9: adding a xHCI host-controller driver. The addition of USB PHY support to the XUSB padctl driver has been dropped. Thierry will be posting those patches later. Given the many compile and run-time dependencies in this series, it is probably best if the first 3 patches are picked up by the relevant maintainers in topic branches so that the remainder of the series can go through the Tegra tree. Tested on Jetson TK1 and Nyan-Big with a variety of USB2.0 and USB3.0 memory sticks and ethernet dongles. This has also been tested, with additional out-of-tree patches, on Tegra132 and Tegra210 based boards. For the series, you can add my ... Reviewed-by: Jon Hunter jonath...@nvidia.com Tested-by: Jon Hunter jonath...@nvidia.com I have tested this on a t124 nyan-big with USB3.0 and USB2.0 devices. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 0/9] Tegra xHCI support
Hi Thierry, On 05/05/15 15:42, Thierry Reding wrote: * PGP Signed by an unknown key On Tue, May 05, 2015 at 03:28:25PM +0100, Jon Hunter wrote: Hi Andrew, On 04/05/15 18:36, Andrew Bresticker wrote: This series adds support for xHCI on NVIDIA Tegra SoCs. This includes: - patches 1, 2, and 3: minor cleanups for mailbox framework and xHCI, - patches 4 and 5: adding an MFD driver for the XUSB cmoplex, - patches 6 and 7: adding a driver for the mailbox used to communicate with the xHCI controller's firmware, and - patches 8 and 9: adding a xHCI host-controller driver. The addition of USB PHY support to the XUSB padctl driver has been dropped. Thierry will be posting those patches later. Given the many compile and run-time dependencies in this series, it is probably best if the first 3 patches are picked up by the relevant maintainers in topic branches so that the remainder of the series can go through the Tegra tree. Tested on Jetson TK1 and Nyan-Big with a variety of USB2.0 and USB3.0 memory sticks and ethernet dongles. This has also been tested, with additional out-of-tree patches, on Tegra132 and Tegra210 based boards. For the series, you can add my ... Reviewed-by: Jon Hunter jonath...@nvidia.com Tested-by: Jon Hunter jonath...@nvidia.com I have tested this on a t124 nyan-big with USB3.0 and USB2.0 devices. How can this be tested without the corresponding USB PHY patches that I'm supposed to be sending out? Also, can anyone point me at patches that add the device tree nodes which this driver makes use of? Andrew has a branch here [1] for testing this series. Yes, obviously testing without your PHY piece (which I should have mentioned), but at least we can test Andrew's changes. Cheers Jon [1] https://github.com/abrestic/linux/tree/tegra-xhci-v8-test -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/8] serial: tegra: handle race condition on uart rx side
From: Shardar Shariff Md smoham...@nvidia.com
The tegra serial driver has two paths through which receive data is
copied up to the tty layer. These are:
1. DMA completion callback
2. UART RX interrupt
A UART RX interrupt occurs for either RX_TIMEOUT (data has been sitting
in the Rx FIFO for more than 4 character times without being read
because there is not enough data to reach the trigger level), End of
Receive Data event (receiver detects that data stops coming in for more
than 4 character times) or a receive error.
In the RX interrupt path, the following happens ...
- All RX DMA transfers are stopped
- Any data in the DMA buffer and RX FIFO are copied up to the tty layer.
- DMA is restarted/primed for the RX path
In the DMA completion callback, the DMA buffer is copied up to the tty
layer but there is no check to see if the RX interrupt could have
occurred between the DMA interrupt firing the the DMA callback running.
Hence, if a RX interrupt was to occur shortly after the DMA completion
interrupt, it is possible that the RX interrupt path has already copied
the DMA buffer before the DMA callback has been called. Therefore, when
the DMA callback is called, if the DMA is already in-progress, then this
indicates that the UART RX interrupt has already occurred and there is
nothing to do in the DMA callback. This race condition can cause
duplicated data to be received.
Signed-off-by: Shardar Shariff Md smoham...@nvidia.com
[jonath...@nvidia.com: Moved async_tx_ack() call to after check to see
if DMA has completed because if the DMA is in progress we do not need
to ACK yet. Changed the print from dev_info to dev_debug. Updated
changelog to add more commentary on the race condition based upon
feedback from author.]
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index a53899c47e60..17d8a08b047b 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -583,10 +583,20 @@ static void tegra_uart_rx_dma_complete(void *args)
struct tty_struct *tty = tty_port_tty_get(tup-uport.state-port);
struct tty_port *port = u-state-port;
unsigned long flags;
+ struct dma_tx_state state;
+ enum dma_status status;
- async_tx_ack(tup-rx_dma_desc);
spin_lock_irqsave(u-lock, flags);
+ status = dmaengine_tx_status(tup-rx_dma_chan, tup-rx_cookie, state);
+
+ if (status == DMA_IN_PROGRESS) {
+ dev_dbg(tup-uport.dev, RX DMA is in progress\n);
+ goto done;
+ }
+
+ async_tx_ack(tup-rx_dma_desc);
+
/* Deactivate flow control to stop sender */
if (tup-rts_active)
set_rts(tup, false);
@@ -607,6 +617,7 @@ static void tegra_uart_rx_dma_complete(void *args)
if (tup-rts_active)
set_rts(tup, true);
+done:
spin_unlock_irqrestore(u-lock, flags);
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/8] serial: tegra: Use unsigned types for RX and TX byte counts
The function tty_insert_flip_string() takes an argument size which is
of type size_t. This is an unsigned type. Update the count,
rx_bytes_requested and tx_bytes_requested in the tegra serial driver to
be unsigned integers so that an unsigned type is passed to
tty_insert_flip_string().
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 17d8a08b047b..a5312e4b6393 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -131,8 +131,8 @@ struct tegra_uart_port {
struct dma_async_tx_descriptor *rx_dma_desc;
dma_cookie_ttx_cookie;
dma_cookie_trx_cookie;
- int tx_bytes_requested;
- int rx_bytes_requested;
+ unsigned inttx_bytes_requested;
+ unsigned intrx_bytes_requested;
};
static void tegra_uart_start_next_tx(struct tegra_uart_port *tup);
@@ -408,7 +408,7 @@ static void tegra_uart_tx_dma_complete(void *args)
struct circ_buf *xmit = tup-uport.state-xmit;
struct dma_tx_state state;
unsigned long flags;
- int count;
+ unsigned int count;
dmaengine_tx_status(tup-tx_dma_chan, tup-rx_cookie, state);
count = tup-tx_bytes_requested - state.residue;
@@ -500,7 +500,7 @@ static void tegra_uart_stop_tx(struct uart_port *u)
struct tegra_uart_port *tup = to_tegra_uport(u);
struct circ_buf *xmit = tup-uport.state-xmit;
struct dma_tx_state state;
- int count;
+ unsigned int count;
if (tup-tx_in_progress != TEGRA_UART_TX_DMA)
return;
@@ -550,7 +550,8 @@ static void tegra_uart_handle_rx_pio(struct tegra_uart_port
*tup,
}
static void tegra_uart_copy_rx_to_tty(struct tegra_uart_port *tup,
- struct tty_port *tty, int count)
+ struct tty_port *tty,
+ unsigned int count)
{
int copied;
@@ -579,7 +580,7 @@ static void tegra_uart_rx_dma_complete(void *args)
{
struct tegra_uart_port *tup = args;
struct uart_port *u = tup-uport;
- int count = tup-rx_bytes_requested;
+ unsigned int count = tup-rx_bytes_requested;
struct tty_struct *tty = tty_port_tty_get(tup-uport.state-port);
struct tty_port *port = u-state-port;
unsigned long flags;
@@ -628,7 +629,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port
*tup,
struct tty_struct *tty = tty_port_tty_get(tup-uport.state-port);
struct tty_port *port = tup-uport.state-port;
struct uart_port *u = tup-uport;
- int count;
+ unsigned int count;
/* Deactivate flow control to stop sender */
if (tup-rts_active)
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 6/8] serial: tegra: Fix cookie used by TX channel
The DMA cookie for the RX channel is being used by the TX channel. Therefore, fix driver to use the correct DMA cookie for the TX channel. Signed-off-by: Jon Hunter jonath...@nvidia.com --- drivers/tty/serial/serial-tegra.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c index a5312e4b6393..987c05665e62 100644 --- a/drivers/tty/serial/serial-tegra.c +++ b/drivers/tty/serial/serial-tegra.c @@ -410,7 +410,7 @@ static void tegra_uart_tx_dma_complete(void *args) unsigned long flags; unsigned int count; - dmaengine_tx_status(tup-tx_dma_chan, tup-rx_cookie, state); + dmaengine_tx_status(tup-tx_dma_chan, tup-tx_cookie, state); count = tup-tx_bytes_requested - state.residue; async_tx_ack(tup-tx_dma_desc); spin_lock_irqsave(tup-uport.lock, flags); -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/8] serial: tegra: Correct delay after TX flush
For all tegra devices (up to t210), there is a hardware issue that
requires software to wait for 32 UART clock periods for the flush
to propagate otherwise TX data could be post. Add a helper function
to wait for N UART clock periods and update delay following FIFO
flush to be 32 UART clock cycles.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 1d5ea3964ee5..9e08d3f07509 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -234,6 +234,22 @@ static void tegra_uart_break_ctl(struct uart_port *u, int
break_ctl)
tup-lcr_shadow = lcr;
}
+/**
+ * tegra_uart_wait_cycle_time: Wait for N UART clock periods
+ *
+ * @tup: Tegra serial port data structure.
+ * @cycles:Number of clock periods to wait.
+ *
+ * Tegra UARTs are clocked at 16X the baud/bit rate and hence the UART
+ * clock speed is 16X the current baud rate.
+ */
+static void tegra_uart_wait_cycle_time(struct tegra_uart_port *tup,
+ unsigned int cycles)
+{
+ if (tup-current_baud)
+ udelay(DIV_ROUND_UP(cycles * 100, tup-current_baud * 16));
+}
+
/* Wait for a symbol-time. */
static void tegra_uart_wait_sym_time(struct tegra_uart_port *tup,
unsigned int syms)
@@ -263,8 +279,12 @@ static void tegra_uart_fifo_reset(struct tegra_uart_port
*tup, u8 fcr_bits)
/* Dummy read to ensure the write is posted */
tegra_uart_read(tup, UART_SCR);
- /* Wait for the flush to propagate. */
- tegra_uart_wait_sym_time(tup, 1);
+ /*
+* For all tegra devices (up to t210), there is a hardware issue that
+* requires software to wait for 32 UART clock periods for the flush
+* to propagate, otherwise data could be lost.
+*/
+ tegra_uart_wait_cycle_time(tup, 32);
}
static int tegra_set_baudrate(struct tegra_uart_port *tup, unsigned int baud)
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/8] serial: tegra: Add delay after enabling FIFO mode
For all tegra devices (up to t210), there is a hardware issue that requires software to wait for 3 UART clock periods after enabling the TX fifo, otherwise data could be lost. Signed-off-by: Jon Hunter jonath...@nvidia.com --- drivers/tty/serial/serial-tegra.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c index 9e08d3f07509..0d9d7ceb1dbb 100644 --- a/drivers/tty/serial/serial-tegra.c +++ b/drivers/tty/serial/serial-tegra.c @@ -885,6 +885,16 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup) tup-fcr_shadow |= TEGRA_UART_TX_TRIG_16B; tegra_uart_write(tup, tup-fcr_shadow, UART_FCR); + /* Dummy read to ensure the write is posted */ + tegra_uart_read(tup, UART_SCR); + + /* +* For all tegra devices (up to t210), there is a hardware issue that +* requires software to wait for 3 UART clock periods after enabling +* the TX fifo, otherwise data could be lost. +*/ + tegra_uart_wait_cycle_time(tup, 3); + /* * Initialize the UART with default configuration * (115200, N, 8, 1) so that the receive DMA buffer may be -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/8] serial: tegra: various fixes
Various fixes for the tegra hsuart driver. Tested on tegra124 nyan-big by opening a serial console on ttyTHS0 and performing simple z-modem transfers. Jon Hunter (6): serial: tegra: Correct delay after TX flush serial: tegra: Add delay after enabling FIFO mode serial: tegra: Use unsigned types for RX and TX byte counts serial: tegra: Fix cookie used by TX channel serial: tegra: Correct shutdown of UARTs serial: tegra: Correct error handling on DMA setup Shardar Shariff Md (2): serial: tegra: check the count and read if any from dma serial: tegra: handle race condition on uart rx side drivers/tty/serial/serial-tegra.c | 128 ++ 1 file changed, 87 insertions(+), 41 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/8] serial: tegra: check the count and read if any from dma
From: Shardar Shariff Md smoham...@nvidia.com
It is only necessary to read data from the dma buffer when the count
value is non-zero and hence, tegra_uart_copy_rx_to_tty() so only be
called when this is the case.
Although, this was being tested for in two places, there is a third
place where this was not tested. However, instead of adding another
if-statement prior to calling tegra_uart_copy_rx_to_tty(), move the test
inside the function.
Signed-off-by: Shardar Shariff Md smoham...@nvidia.com
[jonath...@nvidia.com: Re-worked patch to move the check for the count
value inside the function tegra_uart_copy_rx_to_tty(). Updated
changelog with more commentary.]
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 0d9d7ceb1dbb..a53899c47e60 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -554,6 +554,10 @@ static void tegra_uart_copy_rx_to_tty(struct
tegra_uart_port *tup,
{
int copied;
+ /* If count is zero, then there is no data to be copied */
+ if (!count)
+ return;
+
tup-uport.icount.rx += count;
if (!tty) {
dev_err(tup-uport.dev, No tty port\n);
@@ -588,8 +592,7 @@ static void tegra_uart_rx_dma_complete(void *args)
set_rts(tup, false);
/* If we are here, DMA is stopped */
- if (count)
- tegra_uart_copy_rx_to_tty(tup, port, count);
+ tegra_uart_copy_rx_to_tty(tup, port, count);
tegra_uart_handle_rx_pio(tup, port);
if (tty) {
@@ -626,8 +629,7 @@ static void tegra_uart_handle_rx_dma(struct tegra_uart_port
*tup,
count = tup-rx_bytes_requested - state.residue;
/* If we are here, DMA is stopped */
- if (count)
- tegra_uart_copy_rx_to_tty(tup, port, count);
+ tegra_uart_copy_rx_to_tty(tup, port, count);
tegra_uart_handle_rx_pio(tup, port);
if (tty) {
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 7/8] serial: tegra: Correct shutdown of UARTs
There are two issues in the shutdown path of the UARTs which are:
1. The function tegra_uart_shutdown() calls tegra_uart_flush_buffer()
to stop DMA TX transfers. However, tegra_uart_flush_buffer() is
called after the DMA channels have already been freed and so actually
does nothing.
2. The function that frees the DMA channels
(tegra_uart_dma_channel_free()), unmaps the dma buffer before
freeing the DMA channel and does not ensure the DMA has been
stopped.
Resolve this by fixing the code in tegra_uart_dma_channel_free() to
ensure the DMA is stopped, free the DMA channel and then unmap the DMA
buffer. Finally, remove the unnecessary call to tegra_uart_flush_buffer().
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 987c05665e62..96378da9aefc 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1020,24 +1020,23 @@ scrub:
static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
bool dma_to_memory)
{
- struct dma_chan *dma_chan;
-
if (dma_to_memory) {
+ dmaengine_terminate_all(tup-rx_dma_chan);
+ dma_release_channel(tup-rx_dma_chan);
dma_free_coherent(tup-uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
tup-rx_dma_buf_virt, tup-rx_dma_buf_phys);
- dma_chan = tup-rx_dma_chan;
tup-rx_dma_chan = NULL;
tup-rx_dma_buf_phys = 0;
tup-rx_dma_buf_virt = NULL;
} else {
+ dmaengine_terminate_all(tup-tx_dma_chan);
+ dma_release_channel(tup-tx_dma_chan);
dma_unmap_single(tup-uport.dev, tup-tx_dma_buf_phys,
UART_XMIT_SIZE, DMA_TO_DEVICE);
- dma_chan = tup-tx_dma_chan;
tup-tx_dma_chan = NULL;
tup-tx_dma_buf_phys = 0;
tup-tx_dma_buf_virt = NULL;
}
- dma_release_channel(dma_chan);
}
static int tegra_uart_startup(struct uart_port *u)
@@ -1104,8 +1103,6 @@ static void tegra_uart_shutdown(struct uart_port *u)
tegra_uart_dma_channel_free(tup, true);
tegra_uart_dma_channel_free(tup, false);
free_irq(u-irq, tup);
-
- tegra_uart_flush_buffer(u);
}
static void tegra_uart_enable_ms(struct uart_port *u)
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 8/8] serial: tegra: Correct error handling on DMA setup
Function tegra_uart_dma_channel_allocate() does not check that
dma_map_single() mapped the DMA buffer correctly. Add a check for this
and appropriate error handling.
Furthermore, if dmaengine_slave_config() (called by
tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
instead of just dma_release_channel() if dmaengine_slave_config() fails.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 51 +--
1 file changed, 28 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 96378da9aefc..3b63f103f0c9 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port *tup)
return 0;
}
+static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
+ bool dma_to_memory)
+{
+ if (dma_to_memory) {
+ dmaengine_terminate_all(tup-rx_dma_chan);
+ dma_release_channel(tup-rx_dma_chan);
+ dma_free_coherent(tup-uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
+ tup-rx_dma_buf_virt, tup-rx_dma_buf_phys);
+ tup-rx_dma_chan = NULL;
+ tup-rx_dma_buf_phys = 0;
+ tup-rx_dma_buf_virt = NULL;
+ } else {
+ dmaengine_terminate_all(tup-tx_dma_chan);
+ dma_release_channel(tup-tx_dma_chan);
+ dma_unmap_single(tup-uport.dev, tup-tx_dma_buf_phys,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ tup-tx_dma_chan = NULL;
+ tup-tx_dma_buf_phys = 0;
+ tup-tx_dma_buf_virt = NULL;
+ }
+}
+
static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
bool dma_to_memory)
{
@@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
DMA_TO_DEVICE);
+ if (dma_mapping_error(tup-uport.dev, dma_phys)) {
+ dev_err(tup-uport.dev, dma_map_single tx failed\n);
+ dma_release_channel(dma_chan);
+ return -ENOMEM;
+ }
dma_buf = tup-uport.state-xmit.buf;
}
@@ -1013,32 +1040,10 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
return 0;
scrub:
- dma_release_channel(dma_chan);
+ tegra_uart_dma_channel_free(tup, dma_to_memory);
return ret;
}
-static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
- bool dma_to_memory)
-{
- if (dma_to_memory) {
- dmaengine_terminate_all(tup-rx_dma_chan);
- dma_release_channel(tup-rx_dma_chan);
- dma_free_coherent(tup-uport.dev, TEGRA_UART_RX_DMA_BUFFER_SIZE,
- tup-rx_dma_buf_virt, tup-rx_dma_buf_phys);
- tup-rx_dma_chan = NULL;
- tup-rx_dma_buf_phys = 0;
- tup-rx_dma_buf_virt = NULL;
- } else {
- dmaengine_terminate_all(tup-tx_dma_chan);
- dma_release_channel(tup-tx_dma_chan);
- dma_unmap_single(tup-uport.dev, tup-tx_dma_buf_phys,
- UART_XMIT_SIZE, DMA_TO_DEVICE);
- tup-tx_dma_chan = NULL;
- tup-tx_dma_buf_phys = 0;
- tup-tx_dma_buf_virt = NULL;
- }
-}
-
static int tegra_uart_startup(struct uart_port *u)
{
struct tegra_uart_port *tup = to_tegra_uport(u);
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: mailbox: tegra xusb: undefined MFD_TEGRA_XUSB
Hi Valentin, On 13/05/15 14:22, Valentin Rothberg wrote: Hi Andrew, your commit b1f10002b00a (mailbox: Add NVIDIA Tegra XUSB mailbox driver) is in today's linux-next tree (i.e., next-20150513) and it adds the following lines: +config TEGRA_XUSB_MBOX + tristate NVIDIA Tegra XUSB Mailbox + depends on MFD_TEGRA_XUSB At the current state, the dependency is always false since the option MFD_TEGRA_XUSB is not defined in Kconfig, and the added driver cannot be compiled. Is there a patch queued somewhere to add this missing option? This is part of this series [1] which has the other patches. I don't believe the others in the series have been queued yet. Cheers Jon [1] https://lkml.org/lkml/2015/5/4/574 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup
On 12/05/15 09:39, Alexandre Courbot wrote:
On Tue, May 5, 2015 at 11:17 PM, Jon Hunter jonath...@nvidia.com wrote:
Function tegra_uart_dma_channel_allocate() does not check that
dma_map_single() mapped the DMA buffer correctly. Add a check for this
and appropriate error handling.
Furthermore, if dmaengine_slave_config() (called by
tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
instead of just dma_release_channel() if dmaengine_slave_config() fails.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 51
+--
1 file changed, 28 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 96378da9aefc..3b63f103f0c9 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port
*tup)
return 0;
}
+static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
+ bool dma_to_memory)
+{
+ if (dma_to_memory) {
+ dmaengine_terminate_all(tup-rx_dma_chan);
+ dma_release_channel(tup-rx_dma_chan);
+ dma_free_coherent(tup-uport.dev,
TEGRA_UART_RX_DMA_BUFFER_SIZE,
+ tup-rx_dma_buf_virt, tup-rx_dma_buf_phys);
+ tup-rx_dma_chan = NULL;
+ tup-rx_dma_buf_phys = 0;
+ tup-rx_dma_buf_virt = NULL;
+ } else {
+ dmaengine_terminate_all(tup-tx_dma_chan);
+ dma_release_channel(tup-tx_dma_chan);
+ dma_unmap_single(tup-uport.dev, tup-tx_dma_buf_phys,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ tup-tx_dma_chan = NULL;
+ tup-tx_dma_buf_phys = 0;
+ tup-tx_dma_buf_virt = NULL;
+ }
+}
+
static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
bool dma_to_memory)
{
@@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
DMA_TO_DEVICE);
+ if (dma_mapping_error(tup-uport.dev, dma_phys)) {
+ dev_err(tup-uport.dev, dma_map_single tx
failed\n);
+ dma_release_channel(dma_chan);
+ return -ENOMEM;
Is -ENOMEM the error code we want to return here?
I think that it is appropriate as we are unable to map the memory we are
requesting. I did look at a few other drivers and several return -ENOMEM
here. I saw others return -EFAULT, but given this is memory related,
seems ok, unless you have a better suggestion.
IIUC dma_buf will be leaked if an error occurs here because it has not
been assigned to your structure and will therefore be ignored when
tegra_uart_dma_channel_free() is called.
In the original code, if dmaengine_slave_config() failed, then yes there
would be a memory leak. That should no longer be the case.
Since we have a scrub label at the end of this function, I think I'd
also prefer if this block and the one before could jump to error
labels as well for consistency.
Yes I see. I wondered if it would be better to just get rid of the
scrub label since it is only used in one place instead?
By the way, I got a notification from Greg that these are now queued in
his tty-testing branch [1]. Assuming these are ok, may be I could fix
that up in a follow-up patch?
Otherwise, pretty nice series, comprehensive and easy to read.
Thanks!
Jon
[1]
http://git.kernel.org/cgit/linux/kernel/git/gregkh/tty.git/log/?h=tty-testing
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 4/9] mfd: Add binding document for NVIDIA Tegra XUSB
On 14/05/15 11:23, Lee Jones wrote:
On Thu, 14 May 2015, Jon Hunter wrote:
On 14/05/15 10:30, Lee Jones wrote:
On Thu, 14 May 2015, Jon Hunter wrote:
On 14/05/15 08:40, Lee Jones wrote:
On Thu, 14 May 2015, Jon Hunter wrote:
On 13/05/15 15:39, Lee Jones wrote:
On Mon, 04 May 2015, Andrew Bresticker wrote:
Add a binding document for the XUSB host complex on NVIDIA Tegra124
and later SoCs. The XUSB host complex includes a mailbox for
communication with the XUSB micro-controller and an xHCI
host-controller.
Signed-off-by: Andrew Bresticker abres...@chromium.org
Cc: Rob Herring robh...@kernel.org
Cc: Pawel Moll pawel.m...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Ian Campbell ijc+devicet...@hellion.org.uk
Cc: Kumar Gala ga...@codeaurora.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Lee Jones lee.jo...@linaro.org
---
Changes from v7:
- Move non-shared resources into child nodes.
New for v7.
---
.../bindings/mfd/nvidia,tegra124-xusb.txt | 37
++
1 file changed, 37 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
diff --git
a/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
new file mode 100644
index 000..bc50110
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
@@ -0,0 +1,37 @@
+NVIDIA Tegra XUSB host copmlex
+==
+
+The XUSB host complex on Tegra124 and later SoCs contains an xHCI host
+controller and a mailbox for communication with the XUSB
micro-controller.
+
+Required properties:
+
+ - compatible: For Tegra124, must contain nvidia,tegra124-xusb.
+ Otherwise, must contain 'nvidia,chip-xusb,
nvidia,tegra124-xusb'
+ where chip is tegra132.
+ - reg: Must contain the base and length of the XUSB FPCI registers.
+ - ranges: Bus address mapping for the XUSB block. Can be empty
since the
+ mapping is 1:1.
+ - #address-cells: Must be 2.
+ - #size-cells: Must be 2.
+
+Example:
+
+ usb@0,70098000 {
+ compatible = nvidia,tegra124-xusb;
+ reg = 0x0 0x70098000 0x0 0x1000;
+ ranges;
+
+ #address-cells = 2;
+ #size-cells = 2;
+
+ usb-host@0,7009 {
+ compatible = nvidia,tegra124-xhci;
+ ...
+ };
+
+ mailbox {
+ compatible = nvidia,tegra124-xusb-mbox;
+ ...
+ };
This doesn't appear to be a proper MFD. I would have the USB and
Mailbox devices probe seperately and use a phandle to point the USB
device to its Mailbox.
usb@xyz {
mboxes = xusb-mailbox, [chan];
};
I am assuming that Andrew had laid it out like this to reflect the hw
structure. The mailbox and xhci controller are part of the xusb
sub-system and hence appear as child nodes. My understanding is that for
device-tree we want the device-tree structure to reflect the actual hw.
Is this not the case?
Yes, the DT files should reflect h/w. I have requested to see what
the memory map looks like, so I might provide a more appropriate
solution to accepting a pretty pointless MFD.
For the xusb-host has memory from 0x7009000 - 0x7009.
Within this range, we have this fpci range which is defined as 0x7009800
- 0x70098fff. This range is being shared between the mailbox and xhci
drivers. Looking at the drivers, we have ...
mailbox uses: 0x700980e0 - 0x700980f3 and 0x70098428 - 0x7009842b.
xhci uses:0x70098000 - 0x700980cf and 0x70098800 - 0x70098803.
So it is a bit messy as they overlap. However, we could have ...
xusb_mbox: mailbox {
compatible = nvidia,tegra124-xusb-mbox;
reg = 0x0 0x700980e0 0x0 0x14,
0x0 0x70098428 0x0 0x4;
...
};
usb-host@0,7009 {
compatible = nvidia,tegra124-xhci;
reg = 0x0 0x7009 0x0 0x8000,
0x0 0x70098000 0x0 0x00d0;
0x0 0x70098800 0x0 0x0004;
0x0 0x70099000 0x0 0x1000;
...
};
I believe that Thierry and Stephen said that they wished to avoid
multiple devices sharing the same memory ranges, and so we would need to
divvy up the memory map as above. However, I am not sure if this is an
ok thing to do.
Two solutions spring to mind. You can either call
of_platform_populate() from the USB driver, as some already do:
drivers/usb/dwc3/dwc3-exynos.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-keystone.c:
error = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-omap.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-qcom.c:
ret = of_platform_populate(node, NULL
Re: [PATCH v8 4/9] mfd: Add binding document for NVIDIA Tegra XUSB
On 14/05/15 08:29, Lee Jones wrote:
On Wed, 13 May 2015, Andrew Bresticker wrote:
On Wed, May 13, 2015 at 9:50 AM, Lee Jones lee.jo...@linaro.org wrote:
On Wed, 13 May 2015, Andrew Bresticker wrote:
Lee,
On Wed, May 13, 2015 at 7:39 AM, Lee Jones lee.jo...@linaro.org wrote:
On Mon, 04 May 2015, Andrew Bresticker wrote:
Add a binding document for the XUSB host complex on NVIDIA Tegra124
and later SoCs. The XUSB host complex includes a mailbox for
communication with the XUSB micro-controller and an xHCI host-controller.
Signed-off-by: Andrew Bresticker abres...@chromium.org
Cc: Rob Herring robh...@kernel.org
Cc: Pawel Moll pawel.m...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Ian Campbell ijc+devicet...@hellion.org.uk
Cc: Kumar Gala ga...@codeaurora.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Lee Jones lee.jo...@linaro.org
---
Changes from v7:
- Move non-shared resources into child nodes.
New for v7.
---
.../bindings/mfd/nvidia,tegra124-xusb.txt | 37
++
1 file changed, 37 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
diff --git
a/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
new file mode 100644
index 000..bc50110
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
@@ -0,0 +1,37 @@
+NVIDIA Tegra XUSB host copmlex
+==
+
+The XUSB host complex on Tegra124 and later SoCs contains an xHCI host
+controller and a mailbox for communication with the XUSB
micro-controller.
+
+Required properties:
+
+ - compatible: For Tegra124, must contain nvidia,tegra124-xusb.
+ Otherwise, must contain 'nvidia,chip-xusb,
nvidia,tegra124-xusb'
+ where chip is tegra132.
+ - reg: Must contain the base and length of the XUSB FPCI registers.
+ - ranges: Bus address mapping for the XUSB block. Can be empty since
the
+ mapping is 1:1.
+ - #address-cells: Must be 2.
+ - #size-cells: Must be 2.
+
+Example:
+
+ usb@0,70098000 {
+ compatible = nvidia,tegra124-xusb;
+ reg = 0x0 0x70098000 0x0 0x1000;
+ ranges;
+
+ #address-cells = 2;
+ #size-cells = 2;
+
+ usb-host@0,7009 {
+ compatible = nvidia,tegra124-xhci;
+ ...
+ };
+
+ mailbox {
+ compatible = nvidia,tegra124-xusb-mbox;
+ ...
+ };
This doesn't appear to be a proper MFD. I would have the USB and
Mailbox devices probe seperately and use a phandle to point the USB
device to its Mailbox.
usb@xyz {
mboxes = xusb-mailbox, [chan];
};
Then what will set up the shared regmap? The point of using an MFD
here was to share a register set with both the mailbox and xHCI
drivers and avoid having to map the same register set twice in two
separate drivers.
Can you share the mapping please?
What do you mean? That's what the MFD is doing now by setting up an
MMIO regmap to be shared between the two devices.
I mean, can you share the documentation with me. :)
You can download the documentation from here [1]. See chapter 19 USB
complex. However, I will warn you that unfortunately, you need to sign
up for an account :-(
Cheers
Jon
[1] https://developer.nvidia.com/tegra-k1-technical-reference-manual
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 4/9] mfd: Add binding document for NVIDIA Tegra XUSB
Hi Lee,
On 13/05/15 15:39, Lee Jones wrote:
On Mon, 04 May 2015, Andrew Bresticker wrote:
Add a binding document for the XUSB host complex on NVIDIA Tegra124
and later SoCs. The XUSB host complex includes a mailbox for
communication with the XUSB micro-controller and an xHCI host-controller.
Signed-off-by: Andrew Bresticker abres...@chromium.org
Cc: Rob Herring robh...@kernel.org
Cc: Pawel Moll pawel.m...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Ian Campbell ijc+devicet...@hellion.org.uk
Cc: Kumar Gala ga...@codeaurora.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Lee Jones lee.jo...@linaro.org
---
Changes from v7:
- Move non-shared resources into child nodes.
New for v7.
---
.../bindings/mfd/nvidia,tegra124-xusb.txt | 37
++
1 file changed, 37 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
diff --git a/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
new file mode 100644
index 000..bc50110
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
@@ -0,0 +1,37 @@
+NVIDIA Tegra XUSB host copmlex
+==
+
+The XUSB host complex on Tegra124 and later SoCs contains an xHCI host
+controller and a mailbox for communication with the XUSB micro-controller.
+
+Required properties:
+
+ - compatible: For Tegra124, must contain nvidia,tegra124-xusb.
+ Otherwise, must contain 'nvidia,chip-xusb, nvidia,tegra124-xusb'
+ where chip is tegra132.
+ - reg: Must contain the base and length of the XUSB FPCI registers.
+ - ranges: Bus address mapping for the XUSB block. Can be empty since the
+ mapping is 1:1.
+ - #address-cells: Must be 2.
+ - #size-cells: Must be 2.
+
+Example:
+
+usb@0,70098000 {
+compatible = nvidia,tegra124-xusb;
+reg = 0x0 0x70098000 0x0 0x1000;
+ranges;
+
+#address-cells = 2;
+#size-cells = 2;
+
+usb-host@0,7009 {
+compatible = nvidia,tegra124-xhci;
+...
+};
+
+mailbox {
+compatible = nvidia,tegra124-xusb-mbox;
+...
+};
This doesn't appear to be a proper MFD. I would have the USB and
Mailbox devices probe seperately and use a phandle to point the USB
device to its Mailbox.
usb@xyz {
mboxes = xusb-mailbox, [chan];
};
I am assuming that Andrew had laid it out like this to reflect the hw
structure. The mailbox and xhci controller are part of the xusb
sub-system and hence appear as child nodes. My understanding is that for
device-tree we want the device-tree structure to reflect the actual hw.
Is this not the case?
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 4/9] mfd: Add binding document for NVIDIA Tegra XUSB
On 14/05/15 08:40, Lee Jones wrote:
On Thu, 14 May 2015, Jon Hunter wrote:
Hi Lee,
On 13/05/15 15:39, Lee Jones wrote:
On Mon, 04 May 2015, Andrew Bresticker wrote:
Add a binding document for the XUSB host complex on NVIDIA Tegra124
and later SoCs. The XUSB host complex includes a mailbox for
communication with the XUSB micro-controller and an xHCI host-controller.
Signed-off-by: Andrew Bresticker abres...@chromium.org
Cc: Rob Herring robh...@kernel.org
Cc: Pawel Moll pawel.m...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Ian Campbell ijc+devicet...@hellion.org.uk
Cc: Kumar Gala ga...@codeaurora.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Lee Jones lee.jo...@linaro.org
---
Changes from v7:
- Move non-shared resources into child nodes.
New for v7.
---
.../bindings/mfd/nvidia,tegra124-xusb.txt | 37
++
1 file changed, 37 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
diff --git
a/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
new file mode 100644
index 000..bc50110
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
@@ -0,0 +1,37 @@
+NVIDIA Tegra XUSB host copmlex
+==
+
+The XUSB host complex on Tegra124 and later SoCs contains an xHCI host
+controller and a mailbox for communication with the XUSB micro-controller.
+
+Required properties:
+
+ - compatible: For Tegra124, must contain nvidia,tegra124-xusb.
+ Otherwise, must contain 'nvidia,chip-xusb, nvidia,tegra124-xusb'
+ where chip is tegra132.
+ - reg: Must contain the base and length of the XUSB FPCI registers.
+ - ranges: Bus address mapping for the XUSB block. Can be empty since the
+ mapping is 1:1.
+ - #address-cells: Must be 2.
+ - #size-cells: Must be 2.
+
+Example:
+
+ usb@0,70098000 {
+ compatible = nvidia,tegra124-xusb;
+ reg = 0x0 0x70098000 0x0 0x1000;
+ ranges;
+
+ #address-cells = 2;
+ #size-cells = 2;
+
+ usb-host@0,7009 {
+ compatible = nvidia,tegra124-xhci;
+ ...
+ };
+
+ mailbox {
+ compatible = nvidia,tegra124-xusb-mbox;
+ ...
+ };
This doesn't appear to be a proper MFD. I would have the USB and
Mailbox devices probe seperately and use a phandle to point the USB
device to its Mailbox.
usb@xyz {
mboxes = xusb-mailbox, [chan];
};
I am assuming that Andrew had laid it out like this to reflect the hw
structure. The mailbox and xhci controller are part of the xusb
sub-system and hence appear as child nodes. My understanding is that for
device-tree we want the device-tree structure to reflect the actual hw.
Is this not the case?
Yes, the DT files should reflect h/w. I have requested to see what
the memory map looks like, so I might provide a more appropriate
solution to accepting a pretty pointless MFD.
For the xusb-host has memory from 0x7009000 - 0x7009.
Within this range, we have this fpci range which is defined as 0x7009800
- 0x70098fff. This range is being shared between the mailbox and xhci
drivers. Looking at the drivers, we have ...
mailbox uses: 0x700980e0 - 0x700980f3 and 0x70098428 - 0x7009842b.
xhci uses:0x70098000 - 0x700980cf and 0x70098800 - 0x70098803.
So it is a bit messy as they overlap. However, we could have ...
xusb_mbox: mailbox {
compatible = nvidia,tegra124-xusb-mbox;
reg = 0x0 0x700980e0 0x0 0x14,
0x0 0x70098428 0x0 0x4;
...
};
usb-host@0,7009 {
compatible = nvidia,tegra124-xhci;
reg = 0x0 0x7009 0x0 0x8000,
0x0 0x70098000 0x0 0x00d0;
0x0 0x70098800 0x0 0x0004;
0x0 0x70099000 0x0 0x1000;
...
};
I believe that Thierry and Stephen said that they wished to avoid
multiple devices sharing the same memory ranges, and so we would need to
divvy up the memory map as above. However, I am not sure if this is an
ok thing to do.
Two solutions spring to mind. You can either call
of_platform_populate() from the USB driver, as some already do:
drivers/usb/dwc3/dwc3-exynos.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-keystone.c:
error = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-omap.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-qcom.c:
ret = of_platform_populate(node, NULL, NULL, qdwc-dev);
drivers/usb/dwc3/dwc3-st.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/musb/musb_am335x.c:
ret
Re: [PATCH v8 4/9] mfd: Add binding document for NVIDIA Tegra XUSB
On 14/05/15 10:30, Lee Jones wrote:
On Thu, 14 May 2015, Jon Hunter wrote:
On 14/05/15 08:40, Lee Jones wrote:
On Thu, 14 May 2015, Jon Hunter wrote:
On 13/05/15 15:39, Lee Jones wrote:
On Mon, 04 May 2015, Andrew Bresticker wrote:
Add a binding document for the XUSB host complex on NVIDIA Tegra124
and later SoCs. The XUSB host complex includes a mailbox for
communication with the XUSB micro-controller and an xHCI host-controller.
Signed-off-by: Andrew Bresticker abres...@chromium.org
Cc: Rob Herring robh...@kernel.org
Cc: Pawel Moll pawel.m...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Ian Campbell ijc+devicet...@hellion.org.uk
Cc: Kumar Gala ga...@codeaurora.org
Cc: Samuel Ortiz sa...@linux.intel.com
Cc: Lee Jones lee.jo...@linaro.org
---
Changes from v7:
- Move non-shared resources into child nodes.
New for v7.
---
.../bindings/mfd/nvidia,tegra124-xusb.txt | 37
++
1 file changed, 37 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
diff --git
a/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
new file mode 100644
index 000..bc50110
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/nvidia,tegra124-xusb.txt
@@ -0,0 +1,37 @@
+NVIDIA Tegra XUSB host copmlex
+==
+
+The XUSB host complex on Tegra124 and later SoCs contains an xHCI host
+controller and a mailbox for communication with the XUSB
micro-controller.
+
+Required properties:
+
+ - compatible: For Tegra124, must contain nvidia,tegra124-xusb.
+ Otherwise, must contain 'nvidia,chip-xusb,
nvidia,tegra124-xusb'
+ where chip is tegra132.
+ - reg: Must contain the base and length of the XUSB FPCI registers.
+ - ranges: Bus address mapping for the XUSB block. Can be empty since
the
+ mapping is 1:1.
+ - #address-cells: Must be 2.
+ - #size-cells: Must be 2.
+
+Example:
+
+usb@0,70098000 {
+compatible = nvidia,tegra124-xusb;
+reg = 0x0 0x70098000 0x0 0x1000;
+ranges;
+
+#address-cells = 2;
+#size-cells = 2;
+
+usb-host@0,7009 {
+compatible = nvidia,tegra124-xhci;
+...
+};
+
+mailbox {
+compatible = nvidia,tegra124-xusb-mbox;
+...
+};
This doesn't appear to be a proper MFD. I would have the USB and
Mailbox devices probe seperately and use a phandle to point the USB
device to its Mailbox.
usb@xyz {
mboxes = xusb-mailbox, [chan];
};
I am assuming that Andrew had laid it out like this to reflect the hw
structure. The mailbox and xhci controller are part of the xusb
sub-system and hence appear as child nodes. My understanding is that for
device-tree we want the device-tree structure to reflect the actual hw.
Is this not the case?
Yes, the DT files should reflect h/w. I have requested to see what
the memory map looks like, so I might provide a more appropriate
solution to accepting a pretty pointless MFD.
For the xusb-host has memory from 0x7009000 - 0x7009.
Within this range, we have this fpci range which is defined as 0x7009800
- 0x70098fff. This range is being shared between the mailbox and xhci
drivers. Looking at the drivers, we have ...
mailbox uses: 0x700980e0 - 0x700980f3 and 0x70098428 - 0x7009842b.
xhci uses:0x70098000 - 0x700980cf and 0x70098800 - 0x70098803.
So it is a bit messy as they overlap. However, we could have ...
xusb_mbox: mailbox {
compatible = nvidia,tegra124-xusb-mbox;
reg = 0x0 0x700980e0 0x0 0x14,
0x0 0x70098428 0x0 0x4;
...
};
usb-host@0,7009 {
compatible = nvidia,tegra124-xhci;
reg = 0x0 0x7009 0x0 0x8000,
0x0 0x70098000 0x0 0x00d0;
0x0 0x70098800 0x0 0x0004;
0x0 0x70099000 0x0 0x1000;
...
};
I believe that Thierry and Stephen said that they wished to avoid
multiple devices sharing the same memory ranges, and so we would need to
divvy up the memory map as above. However, I am not sure if this is an
ok thing to do.
Two solutions spring to mind. You can either call
of_platform_populate() from the USB driver, as some already do:
drivers/usb/dwc3/dwc3-exynos.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-keystone.c:
error = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-omap.c:
ret = of_platform_populate(node, NULL, NULL, dev);
drivers/usb/dwc3/dwc3-qcom.c:
ret = of_platform_populate(node, NULL, NULL, qdwc-dev);
drivers
Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup
On 13/05/15 05:56, Alexandre Courbot wrote:
On Tue, May 12, 2015 at 6:51 PM, Jon Hunter jonath...@nvidia.com wrote:
On 12/05/15 09:39, Alexandre Courbot wrote:
On Tue, May 5, 2015 at 11:17 PM, Jon Hunter jonath...@nvidia.com wrote:
Function tegra_uart_dma_channel_allocate() does not check that
dma_map_single() mapped the DMA buffer correctly. Add a check for this
and appropriate error handling.
Furthermore, if dmaengine_slave_config() (called by
tegra_uart_dma_channel_allocate()) fails, then memory allocated/mapped
is not freed/unmapped. Therefore, call tegra_uart_dma_channel_free()
instead of just dma_release_channel() if dmaengine_slave_config() fails.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 51
+--
1 file changed, 28 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 96378da9aefc..3b63f103f0c9 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -949,6 +949,28 @@ static int tegra_uart_hw_init(struct tegra_uart_port
*tup)
return 0;
}
+static void tegra_uart_dma_channel_free(struct tegra_uart_port *tup,
+ bool dma_to_memory)
+{
+ if (dma_to_memory) {
+ dmaengine_terminate_all(tup-rx_dma_chan);
+ dma_release_channel(tup-rx_dma_chan);
+ dma_free_coherent(tup-uport.dev,
TEGRA_UART_RX_DMA_BUFFER_SIZE,
+ tup-rx_dma_buf_virt,
tup-rx_dma_buf_phys);
+ tup-rx_dma_chan = NULL;
+ tup-rx_dma_buf_phys = 0;
+ tup-rx_dma_buf_virt = NULL;
+ } else {
+ dmaengine_terminate_all(tup-tx_dma_chan);
+ dma_release_channel(tup-tx_dma_chan);
+ dma_unmap_single(tup-uport.dev, tup-tx_dma_buf_phys,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ tup-tx_dma_chan = NULL;
+ tup-tx_dma_buf_phys = 0;
+ tup-tx_dma_buf_virt = NULL;
+ }
+}
+
static int tegra_uart_dma_channel_allocate(struct tegra_uart_port *tup,
bool dma_to_memory)
{
@@ -981,6 +1003,11 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
DMA_TO_DEVICE);
+ if (dma_mapping_error(tup-uport.dev, dma_phys)) {
+ dev_err(tup-uport.dev, dma_map_single tx
failed\n);
+ dma_release_channel(dma_chan);
+ return -ENOMEM;
Is -ENOMEM the error code we want to return here?
I think that it is appropriate as we are unable to map the memory we are
requesting. I did look at a few other drivers and several return -ENOMEM
here. I saw others return -EFAULT, but given this is memory related,
seems ok, unless you have a better suggestion.
IIUC dma_buf will be leaked if an error occurs here because it has not
been assigned to your structure and will therefore be ignored when
tegra_uart_dma_channel_free() is called.
In the original code, if dmaengine_slave_config() failed, then yes there
would be a memory leak. That should no longer be the case.
Mmm I am pretty sure that even after your patch the memory allocated
through the DMA API will not be freed if we hit an error there,
because dma_buf/dma_phys are not yet affected to your tegra_uart_port
structure when you call dma_release_channel(). Or maybe I am missing
something?
So there are two paths through the tegra_uart_dma_channel_allocate()
function, one for RX and one for TX. In the RX case, a buffer is
allocated via dma_alloc_coherent() and if this fails, then we simply
call dma_release_channel(). So there should not be any memory leaked in
this path and we should not need to worry about dma_buf/dma_phys here.
In the TX case, the xmit.buf (allocated by the serial_core driver) is
mapped to physical space for DMA. If the mapping fails, the xmit.buf is
not freed here and we simply call dma_release_channel().
If you are concerned about the xmit.buf, then this is part serial core
and allocated when uart_open() is called. It uart_open() fails, because
the tegra_uart_dma_channel_allocate() fails, then uart_close() will be
called (according the to kernel-doc for uart_open) and should be freed
when uart_shutdown() is called. So I don't see a problem here.
Let me know if I am misunderstanding you.
Since we have a scrub label at the end of this function, I think I'd
also prefer if this block and the one before could jump to error
labels as well for consistency.
Yes I see. I wondered if it would be better to just get rid of the
scrub label since it is only used in one place instead?
I am fine with whichever makes the most sense, although I am biased
Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup
On 20/05/15 04:57, Alexandre Courbot wrote:
[snip]
There may still be a leak (that is not related to your patch) in the
RX path though:
dma_buf = dma_alloc_coherent(...);
ret = dmaengine_slave_config(...);
if (ret 0) {
...
goto scrub;
}
tup-rx_dma_buf_virt = dma_buf;
tup-rx_dma_buf_phys = dma_phys;
scrub:
dma_release_channel(dma_chan);
return ret;
It seems that if dmaengine_slave_config() fails, then the result of
dma_alloc_coherent() remains purely local to the function and is never
freed. Or am I missing something again?
Yes, you are right here. Actually, this would be applicable to
both RX and TX paths because in the TX path the buffer would
not be unmapped. I will generated a separate patch for fix
this. How does the following look?
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 3b63f103f0c9..cf0133ae762d 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_release_channel(dma_chan);
return -ENOMEM;
}
+ dma_sconfig.src_addr = tup-uport.mapbase;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_maxburst = 4;
+ tup-rx_dma_chan = dma_chan;
+ tup-rx_dma_buf_virt = dma_buf;
+ tup-rx_dma_buf_phys = dma_phys;
} else {
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
@@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
return -ENOMEM;
}
dma_buf = tup-uport.state-xmit.buf;
- }
-
- if (dma_to_memory) {
- dma_sconfig.src_addr = tup-uport.mapbase;
- dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- dma_sconfig.src_maxburst = 4;
- } else {
dma_sconfig.dst_addr = tup-uport.mapbase;
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 16;
+ tup-tx_dma_chan = dma_chan;
+ tup-tx_dma_buf_virt = dma_buf;
+ tup-tx_dma_buf_phys = dma_phys;
}
ret = dmaengine_slave_config(dma_chan, dma_sconfig);
if (ret 0) {
dev_err(tup-uport.dev,
Dma slave config failed, err = %d\n, ret);
- goto scrub;
+ tegra_uart_dma_channel_free(tup, dma_to_memory);
+ return ret;
}
- if (dma_to_memory) {
- tup-rx_dma_chan = dma_chan;
- tup-rx_dma_buf_virt = dma_buf;
- tup-rx_dma_buf_phys = dma_phys;
- } else {
- tup-tx_dma_chan = dma_chan;
- tup-tx_dma_buf_virt = dma_buf;
- tup-tx_dma_buf_phys = dma_phys;
- }
return 0;
-
-scrub:
- tegra_uart_dma_channel_free(tup, dma_to_memory);
- return ret;
}
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] serial: tegra: Fix memory leak on DMA setup failure
If the call to dmaengine_slave_config() fails, then the DMA buffer will
not be freed/unmapped. Fix this by moving the code that stores the
address of the buffer in the tegra_uart_port structure to before the
call to dmaengine_slave_config().
Reported-by: Alexandre Courbot acour...@nvidia.com
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 32 +++-
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 3b63f103f0c9..cf0133ae762d 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_release_channel(dma_chan);
return -ENOMEM;
}
+ dma_sconfig.src_addr = tup-uport.mapbase;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_maxburst = 4;
+ tup-rx_dma_chan = dma_chan;
+ tup-rx_dma_buf_virt = dma_buf;
+ tup-rx_dma_buf_phys = dma_phys;
} else {
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
@@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
return -ENOMEM;
}
dma_buf = tup-uport.state-xmit.buf;
- }
-
- if (dma_to_memory) {
- dma_sconfig.src_addr = tup-uport.mapbase;
- dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- dma_sconfig.src_maxburst = 4;
- } else {
dma_sconfig.dst_addr = tup-uport.mapbase;
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 16;
+ tup-tx_dma_chan = dma_chan;
+ tup-tx_dma_buf_virt = dma_buf;
+ tup-tx_dma_buf_phys = dma_phys;
}
ret = dmaengine_slave_config(dma_chan, dma_sconfig);
if (ret 0) {
dev_err(tup-uport.dev,
Dma slave config failed, err = %d\n, ret);
- goto scrub;
+ tegra_uart_dma_channel_free(tup, dma_to_memory);
+ return ret;
}
- if (dma_to_memory) {
- tup-rx_dma_chan = dma_chan;
- tup-rx_dma_buf_virt = dma_buf;
- tup-rx_dma_buf_phys = dma_phys;
- } else {
- tup-tx_dma_chan = dma_chan;
- tup-tx_dma_buf_virt = dma_buf;
- tup-tx_dma_buf_phys = dma_phys;
- }
return 0;
-
-scrub:
- tegra_uart_dma_channel_free(tup, dma_to_memory);
- return ret;
}
static int tegra_uart_startup(struct uart_port *u)
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial: tegra: Fix memory leak on DMA setup failure
On 20/05/15 12:21, Jon Hunter wrote:
If the call to dmaengine_slave_config() fails, then the DMA buffer will
not be freed/unmapped. Fix this by moving the code that stores the
address of the buffer in the tegra_uart_port structure to before the
call to dmaengine_slave_config().
By the way, just to be clear, I did try to fix this before [1], but
failed :-(
Thanks to Alex for pointing this out.
Cheers
Jon
[1] https://lkml.org/lkml/2015/5/5/802
Reported-by: Alexandre Courbot acour...@nvidia.com
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/tty/serial/serial-tegra.c | 32 +++-
1 file changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 3b63f103f0c9..cf0133ae762d 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_release_channel(dma_chan);
return -ENOMEM;
}
+ dma_sconfig.src_addr = tup-uport.mapbase;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_maxburst = 4;
+ tup-rx_dma_chan = dma_chan;
+ tup-rx_dma_buf_virt = dma_buf;
+ tup-rx_dma_buf_phys = dma_phys;
} else {
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
@@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
return -ENOMEM;
}
dma_buf = tup-uport.state-xmit.buf;
- }
-
- if (dma_to_memory) {
- dma_sconfig.src_addr = tup-uport.mapbase;
- dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- dma_sconfig.src_maxburst = 4;
- } else {
dma_sconfig.dst_addr = tup-uport.mapbase;
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 16;
+ tup-tx_dma_chan = dma_chan;
+ tup-tx_dma_buf_virt = dma_buf;
+ tup-tx_dma_buf_phys = dma_phys;
}
ret = dmaengine_slave_config(dma_chan, dma_sconfig);
if (ret 0) {
dev_err(tup-uport.dev,
Dma slave config failed, err = %d\n, ret);
- goto scrub;
+ tegra_uart_dma_channel_free(tup, dma_to_memory);
+ return ret;
}
- if (dma_to_memory) {
- tup-rx_dma_chan = dma_chan;
- tup-rx_dma_buf_virt = dma_buf;
- tup-rx_dma_buf_phys = dma_phys;
- } else {
- tup-tx_dma_chan = dma_chan;
- tup-tx_dma_buf_virt = dma_buf;
- tup-tx_dma_buf_phys = dma_phys;
- }
return 0;
-
-scrub:
- tegra_uart_dma_channel_free(tup, dma_to_memory);
- return ret;
}
static int tegra_uart_startup(struct uart_port *u)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial: tegra: Fix memory leak on DMA setup failure
On 20/05/15 12:25, Jon Hunter wrote: On 20/05/15 12:21, Jon Hunter wrote: If the call to dmaengine_slave_config() fails, then the DMA buffer will not be freed/unmapped. Fix this by moving the code that stores the address of the buffer in the tegra_uart_port structure to before the call to dmaengine_slave_config(). By the way, just to be clear, I did try to fix this before [1], but failed :-( To be doubly clear, this is targeted to be applied on top of the previous patch [1] which is now in linux-next. Jon [1] https://lkml.org/lkml/2015/5/5/802 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8] serial: tegra: Correct error handling on DMA setup
On 20/05/15 10:51, Jon Hunter wrote:
On 20/05/15 04:57, Alexandre Courbot wrote:
[snip]
There may still be a leak (that is not related to your patch) in the
RX path though:
dma_buf = dma_alloc_coherent(...);
ret = dmaengine_slave_config(...);
if (ret 0) {
...
goto scrub;
}
tup-rx_dma_buf_virt = dma_buf;
tup-rx_dma_buf_phys = dma_phys;
scrub:
dma_release_channel(dma_chan);
return ret;
It seems that if dmaengine_slave_config() fails, then the result of
dma_alloc_coherent() remains purely local to the function and is never
freed. Or am I missing something again?
Yes, you are right here. Actually, this would be applicable to
both RX and TX paths because in the TX path the buffer would
not be unmapped. I will generated a separate patch for fix
this. How does the following look?
diff --git a/drivers/tty/serial/serial-tegra.c
b/drivers/tty/serial/serial-tegra.c
index 3b63f103f0c9..cf0133ae762d 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -999,6 +999,12 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
dma_release_channel(dma_chan);
return -ENOMEM;
}
+ dma_sconfig.src_addr = tup-uport.mapbase;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_maxburst = 4;
+ tup-rx_dma_chan = dma_chan;
+ tup-rx_dma_buf_virt = dma_buf;
+ tup-rx_dma_buf_phys = dma_phys;
} else {
dma_phys = dma_map_single(tup-uport.dev,
tup-uport.state-xmit.buf, UART_XMIT_SIZE,
@@ -1009,39 +1015,23 @@ static int tegra_uart_dma_channel_allocate(struct
tegra_uart_port *tup,
return -ENOMEM;
}
dma_buf = tup-uport.state-xmit.buf;
- }
-
- if (dma_to_memory) {
- dma_sconfig.src_addr = tup-uport.mapbase;
- dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- dma_sconfig.src_maxburst = 4;
- } else {
dma_sconfig.dst_addr = tup-uport.mapbase;
dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
dma_sconfig.dst_maxburst = 16;
+ tup-tx_dma_chan = dma_chan;
+ tup-tx_dma_buf_virt = dma_buf;
+ tup-tx_dma_buf_phys = dma_phys;
}
ret = dmaengine_slave_config(dma_chan, dma_sconfig);
if (ret 0) {
dev_err(tup-uport.dev,
Dma slave config failed, err = %d\n, ret);
- goto scrub;
+ tegra_uart_dma_channel_free(tup, dma_to_memory);
+ return ret;
}
- if (dma_to_memory) {
- tup-rx_dma_chan = dma_chan;
- tup-rx_dma_buf_virt = dma_buf;
- tup-rx_dma_buf_phys = dma_phys;
- } else {
- tup-tx_dma_chan = dma_chan;
- tup-tx_dma_buf_virt = dma_buf;
- tup-tx_dma_buf_phys = dma_phys;
- }
return 0;
-
-scrub:
- tegra_uart_dma_channel_free(tup, dma_to_memory);
- return ret;
}
Posted the patch here [1].
Jon
[1] https://lkml.org/lkml/2015/5/20/347
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
On 05/06/15 00:02, Paul Walmsley wrote: Hi folks just a brief comment on this one: On Thu, 30 Apr 2015, Boris Brezillon wrote: Clock rates are stored in an unsigned long field, but -round_rate() (which returns a rounded rate from a requested one) returns a long value (errors are reported using negative error codes), which can lead to long overflow if the clock rate exceed 2Ghz. Change -round_rate() prototype to return 0 or an error code, and pass the requested rate as a pointer so that it can be adjusted depending on hardware capabilities. ... diff --git a/Documentation/clk.txt b/Documentation/clk.txt index 0e4f90a..fca8b7a 100644 --- a/Documentation/clk.txt +++ b/Documentation/clk.txt @@ -68,8 +68,8 @@ the operations defined in clk.h: int (*is_enabled)(struct clk_hw *hw); unsigned long (*recalc_rate)(struct clk_hw *hw, unsigned long parent_rate); -long(*round_rate)(struct clk_hw *hw, -unsigned long rate, +int (*round_rate)(struct clk_hw *hw, +unsigned long *rate, unsigned long *parent_rate); long(*determine_rate)(struct clk_hw *hw, unsigned long rate, I'd suggest that we should probably go straight to 64-bit rates. There are already plenty of clock sources that can generate rates higher than 4GiHz. An alternative would be to introduce to a frequency base the default could be Hz (for backwards compatibility), but for CPUs we probably only care about MHz (or may be kHz) and so 32-bits would still suffice. Even if CPUs cared about Hz they could still use Hz, but in that case they probably don't care about GHz. Obviously, we don't want to break DT compatibility but may be the frequency base could be defined in DT and if it is missing then Hz is assumed. Just a thought ... Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
Hi Boris, On 05/06/15 12:39, Boris Brezillon wrote: Hi Jon, On Fri, 5 Jun 2015 09:46:09 +0100 Jon Hunter jonath...@nvidia.com wrote: On 05/06/15 00:02, Paul Walmsley wrote: Hi folks just a brief comment on this one: On Thu, 30 Apr 2015, Boris Brezillon wrote: Clock rates are stored in an unsigned long field, but -round_rate() (which returns a rounded rate from a requested one) returns a long value (errors are reported using negative error codes), which can lead to long overflow if the clock rate exceed 2Ghz. Change -round_rate() prototype to return 0 or an error code, and pass the requested rate as a pointer so that it can be adjusted depending on hardware capabilities. ... diff --git a/Documentation/clk.txt b/Documentation/clk.txt index 0e4f90a..fca8b7a 100644 --- a/Documentation/clk.txt +++ b/Documentation/clk.txt @@ -68,8 +68,8 @@ the operations defined in clk.h: int (*is_enabled)(struct clk_hw *hw); unsigned long (*recalc_rate)(struct clk_hw *hw, unsigned long parent_rate); - long(*round_rate)(struct clk_hw *hw, - unsigned long rate, + int (*round_rate)(struct clk_hw *hw, + unsigned long *rate, unsigned long *parent_rate); long(*determine_rate)(struct clk_hw *hw, unsigned long rate, I'd suggest that we should probably go straight to 64-bit rates. There are already plenty of clock sources that can generate rates higher than 4GiHz. An alternative would be to introduce to a frequency base the default could be Hz (for backwards compatibility), but for CPUs we probably only care about MHz (or may be kHz) and so 32-bits would still suffice. Even if CPUs cared about Hz they could still use Hz, but in that case they probably don't care about GHz. Obviously, we don't want to break DT compatibility but may be the frequency base could be defined in DT and if it is missing then Hz is assumed. Just a thought ... Yes, but is it really worth the additional complexity. You'll have to add the unit information anyway, so using an unsigned long for the value and another field for the unit (an enum ?) is just like using a 64 bit integer. For a storage perspective, yes it would be the same. However, there are probably a lot of devices that would not need the extra range, but would now have to deal with 64-bit types. I have no idea how much overhead that would be in reality. If the overhead is negligible then a 64-bit type is definitely the way to go, as I agree it is simpler and cleaner. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] ARM: tegra124: pmu support
On 15/06/15 19:46, Kyle Huey wrote:
This patch modifies the device tree for tegra124 based devices to enable the
Cortex A15 PMU. The interrupt numbers are taken from NVIDIA TRM
DP-06905-001_v03p. This patch was tested on a Jetson TK1.
Signed-off-by: Kyle Huey kh...@kylehuey.com
---
arch/arm/boot/dts/tegra124.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 4be06c6..d966d4e 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -906,16 +906,24 @@
cpu@3 {
device_type = cpu;
compatible = arm,cortex-a15;
reg = 3;
};
};
+ pmu {
+ compatible = arm,cortex-a15-pmu;
+ interrupts = GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH,
+ GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH,
+ GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH,
+ GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH;
+ };
+
thermal-zones {
cpu {
polling-delay-passive = 1000;
polling-delay = 1000;
thermal-sensors =
soctherm TEGRA124_SOCTHERM_SENSOR_CPU;
};
Acked-by: Jon Hunter jonath...@nvidia.com
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] DMA: tegra-apb: Clean-up
Any feedback on this series? Cheers Jon On 06/08/15 14:32, Jon Hunter wrote: Some clean-up changes for the tegra-apb DMA driver. These have been compile and boot tested for ARM and ARM64. Summary of the ARM results are below. ARM64 has been tested locally. Test summary Build: zImage: Pass: ( 2/ 2): multi_v7_defconfig, tegra_defconfig Boot to userspace: multi_v7_defconfig: Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1, tegra20-trimslice, tegra30-beaver Boot to userspace: tegra_defconfig: Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1, tegra20-trimslice, tegra30-beaver Jon Hunter (4): DMA: tegra-apb: Remove unused variables DMA: tegra-apb: Avoid unnecessary channel base address calculation DMA: tegra-apb: Remove unnecessary return statements and variables DMA: tegra-apb: Simplify locking for device using global pause drivers/dma/tegra20-apb-dma.c | 63 ++- 1 file changed, 38 insertions(+), 25 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] clk: tegra: remove TEGRA_PLL_USE_LOCK for PLLD/PLLD2
From: Vince Hsu vin...@nvidia.com
Tegra114 has a HW bug where the PLLD/PLLD2 lock bit cannot be asserted
while turning on the Display power domain and before the clamp to this
domain has been removed. This issue causes a timeout and aborts the power
up sequence, even though the PLLD/PLLD2 has already locked. To avoid this,
don't use the lock for PLLD/PLLD2, just wait 1ms and treat the clocks as
locked.
Signed-off-by: Vince Hsu vin...@nvidia.com
[jonath...@nvidia.com: Updated the changelog description]
Signed-off-by: Jon Hunter jonath...@nvidia.com
Acked-by: Peter De Schrijver pdeschrij...@nvidia.com
---
I had originally sent this as part of the tegra generic power-domain series
[0]. However, given that this is really a standalone fix and the
power-domain work is on-going, I don't see any reason why this should not
be merged now.
[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356060.html
drivers/clk/tegra/clk-tegra114.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
index db5871519bf5..6c824e469a1b 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -454,8 +454,7 @@ static struct tegra_clk_pll_params pll_d_params = {
.lock_delay = 1000,
.div_nmp = pllp_nmp,
.freq_table = pll_d_freq_table,
- .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON |
-TEGRA_PLL_USE_LOCK,
+ .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON,
};
static struct tegra_clk_pll_params pll_d2_params = {
@@ -472,8 +471,7 @@ static struct tegra_clk_pll_params pll_d2_params = {
.lock_delay = 1000,
.div_nmp = pllp_nmp,
.freq_table = pll_d_freq_table,
- .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON |
-TEGRA_PLL_USE_LOCK,
+ .flags = TEGRA_PLL_HAS_CPCON | TEGRA_PLL_SET_LFCON,
};
static struct pdiv_map pllu_p[] = {
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 1/7] DMA: tegra-apb: Correct runtime-pm usage
On 24/08/15 10:22, Vinod Koul wrote: On Mon, Aug 24, 2015 at 09:47:13AM +0100, Jon Hunter wrote: On 23/08/15 15:17, Vinod Koul wrote: On Tue, Aug 18, 2015 at 02:49:09PM +0100, Jon Hunter wrote: @@ -1543,7 +1531,7 @@ static int tegra_dma_pm_suspend(struct device *dev) int ret; /* Enable clock before accessing register */ - ret = tegra_dma_runtime_resume(dev); + ret = pm_runtime_get_sync(dev); why is this required ? Because the clock could be disabled when this function is called. This function saves the DMA context so that if the context is lost during suspend, it can be restored. Have you verified this? Coz my understanding is that when PM does suspend it will esnure you are runtime resume if runtime suspended and then will do suspend. So you do not need to do above I see what you are saying. I did some testing with ftrace today to trace rpm and suspend/resume calls. If the dma controller is runtime suspended and I do not call pm_runtime_get_sync() above then I do not see any runtime resume of the dma controller prior to suspend. Now I was hoping that this would cause a complete kernel crash but it did not and so the DMA clock did not appear to be needed here (at least on the one board I tested). However, I would not go as far as to remove this and prefer to keep as above. Furthermore, other drivers do similar things, including the sirf dma controller (see sirf-dma.c). Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 6/7] Documentation: DT: Add binding documentation for NVIDIA ADMA
Add device-tree binding documentation for the Tegra210 Audio DMA
controller.
Cc: Rob Herring robh...@kernel.org
Cc: Pawel Moll pawel.m...@arm.com
Cc: Mark Rutland mark.rutl...@arm.com
Cc: Ian Campbell ijc+devicet...@hellion.org.uk
Cc: Kumar Gala ga...@codeaurora.org
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
.../devicetree/bindings/dma/tegra210-adma.txt | 49 ++
1 file changed, 49 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt
diff --git a/Documentation/devicetree/bindings/dma/tegra210-adma.txt
b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
new file mode 100644
index ..38310d7e7e77
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/tegra210-adma.txt
@@ -0,0 +1,49 @@
+* NVIDIA Tegra Audio DMA controller
+
+Required properties:
+- compatible: Should be nvidia,chip-adma
+- reg: Should contain DMA registers location and length. This should include
+ all of the per-channel registers.
+- interrupt-parent: Phandle to the interrupt parent controller.
+- interrupts: Should contain all of the per-channel DMA interrupts.
+- clocks: Must contain two entries, one for the power-domain clock and one
+ for the module clock.
+ See ../clocks/clock-bindings.txt for details.
+- #dma-cells : Must be 1. This dictates the length of DMA specifiers in
+ client nodes' dmas properties. The specifier represents the DMA request
+ select value for the peripheral. For more details, consult the Tegra TRM's
+ documentation of the APB DMA channel control register REQ_SEL field.
+
+Examples:
+
+adma: adma@702e2000 {
+ compatible = nvidia,tegra210-adma;
+ reg = 0x0 0x702e2000 0x0 0x2000;
+ interrupt-parent = tegra_agic;
+ interrupts = GIC_SPI INT_ADMA_EOT0 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT1 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT2 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT3 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT4 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT5 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT6 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT7 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT8 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT9 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT10 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT11 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT12 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT13 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT14 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT15 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT16 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT17 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT18 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT19 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT20 IRQ_TYPE_LEVEL_HIGH,
+GIC_SPI INT_ADMA_EOT21 IRQ_TYPE_LEVEL_HIGH;
+ clocks = tegra_car TEGRA210_CLK_D_AUDIO,
+tegra_car TEGRA210_CLK_ADMA_APE;
+ clock-names = adma, adma.ape;
+ #dma-cells = 1;
+};
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 5/7] DMA: tegra-apb: Move common code into separate source files
Move code that is common between the Tegra20-APB DMA and Tegra210 ADMA
driver into separate source files.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/dma/Kconfig | 4 +
drivers/dma/Makefile | 1 +
drivers/dma/tegra-common.c| 733 ++
drivers/dma/tegra-common.h| 226 +++
drivers/dma/tegra20-apb-dma.c | 910 +-
5 files changed, 972 insertions(+), 902 deletions(-)
create mode 100644 drivers/dma/tegra-common.c
create mode 100644 drivers/dma/tegra-common.h
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index ff50af3f1bb0..dd79b0bf0876 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -189,10 +189,14 @@ config TXX9_DMAC
Support the TXx9 SoC internal DMA controller. This can be
integrated in chips such as the Toshiba TX4927/38/39.
+config TEGRA_DMA_COMMON
+ bool
+
config TEGRA20_APB_DMA
bool NVIDIA Tegra20 APB DMA support
depends on ARCH_TEGRA
select DMA_ENGINE
+ select TEGRA_DMA_COMMON
help
Support for the NVIDIA Tegra20 APB DMA controller driver. The
DMA controller is having multiple DMA channel which can be
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 264eb3c52446..d9c2bf5ef0bd 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
obj-$(CONFIG_TI_EDMA) += edma.o
obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
+obj-$(CONFIG_TEGRA_DMA_COMMON) += tegra-common.o
obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
obj-$(CONFIG_PL330_DMA) += pl330.o
obj-$(CONFIG_PCH_DMA) += pch_dma.o
diff --git a/drivers/dma/tegra-common.c b/drivers/dma/tegra-common.c
new file mode 100644
index ..fff0a143f5bb
--- /dev/null
+++ b/drivers/dma/tegra-common.c
@@ -0,0 +1,733 @@
+/*
+ * Helper functions for NVIDIA DMA drivers.
+ *
+ * Copyright (c) 2015, NVIDIA CORPORATION. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include linux/delay.h
+#include linux/dmaengine.h
+#include linux/interrupt.h
+#include linux/pm_runtime.h
+#include linux/slab.h
+
+#include dmaengine.h
+#include tegra-common.h
+
+static dma_cookie_t tegra_dma_tx_submit(struct dma_async_tx_descriptor *txd)
+{
+ struct tegra_dma_desc *dma_desc = txd_to_tegra_dma_desc(txd);
+ struct tegra_dma_channel *tdc = to_tegra_dma_chan(txd-chan);
+ unsigned long flags;
+ dma_cookie_t cookie;
+
+ spin_lock_irqsave(tdc-lock, flags);
+ dma_desc-dma_status = DMA_IN_PROGRESS;
+ cookie = dma_cookie_assign(dma_desc-txd);
+ list_splice_tail_init(dma_desc-tx_list, tdc-pending_sg_req);
+ spin_unlock_irqrestore(tdc-lock, flags);
+ return cookie;
+}
+
+/* Get DMA desc from free list, if not there then allocate it. */
+static struct tegra_dma_desc *tegra_dma_desc_get(struct tegra_dma_channel *tdc)
+{
+ struct tegra_dma_desc *dma_desc;
+ unsigned long flags;
+
+ spin_lock_irqsave(tdc-lock, flags);
+
+ /* Do not allocate if desc are waiting for ack */
+ list_for_each_entry(dma_desc, tdc-free_dma_desc, node) {
+ if (async_tx_test_ack(dma_desc-txd)) {
+ list_del(dma_desc-node);
+ spin_unlock_irqrestore(tdc-lock, flags);
+ dma_desc-txd.flags = 0;
+ return dma_desc;
+ }
+ }
+
+ spin_unlock_irqrestore(tdc-lock, flags);
+
+ /* Allocate DMA desc */
+ dma_desc = kzalloc(sizeof(*dma_desc), GFP_ATOMIC);
+ if (!dma_desc)
+ return NULL;
+
+ dma_async_tx_descriptor_init(dma_desc-txd, tdc-dma_chan);
+ dma_desc-txd.tx_submit = tegra_dma_tx_submit;
+ dma_desc-txd.flags = 0;
+ return dma_desc;
+}
+
+static void tegra_dma_desc_put(struct tegra_dma_channel *tdc,
+ struct tegra_dma_desc *dma_desc)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(tdc-lock, flags);
+ if (!list_empty(dma_desc-tx_list))
+ list_splice_init(dma_desc-tx_list, tdc-free_sg_req);
+ list_add_tail(dma_desc-node, tdc-free_dma_desc);
+ spin_unlock_irqrestore(tdc-lock, flags);
+}
+
+static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
+ struct tegra_dma_channel *tdc)
+{
+ struct tegra_dma_sg_req *sg_req = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(tdc
[RFC PATCH 3/7] DMA: tegra-apb: Clean-up and simplify setting up of transfer parameters
Most of the DMA transfer parameters that are configured for scatter-gather
or cyclic transfers are the same. Therefore, move the setup of common
parameters into the tegra_dma_get_xfer_params() function used for both
scatter-gather and cyclic transfers.
Note that TEGRA_APBDMA_AHBSEQ_WRAP_NONE is defined as 0 and so this setting
can be completely removed.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/dma/tegra20-apb-dma.c | 53 ---
1 file changed, 19 insertions(+), 34 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index e552a4efef71..c1eb25075756 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -940,7 +940,8 @@ static inline int get_burst_size(struct tegra_dma_channel
*tdc,
static int tegra_dma_get_xfer_params(struct tegra_dma_channel *tdc,
struct tegra_dma_channel_regs *ch_regs,
-enum dma_transfer_direction direction)
+enum dma_transfer_direction direction,
+unsigned int flags)
{
switch (direction) {
case DMA_MEM_TO_DEV:
@@ -948,48 +949,32 @@ static int tegra_dma_get_xfer_params(struct
tegra_dma_channel *tdc,
ch_regs-apb_seq = get_bus_width(tdc,
tdc-dma_sconfig.dst_addr_width);
ch_regs-csr = TEGRA_APBDMA_CSR_DIR;
- return 0;
+ break;
case DMA_DEV_TO_MEM:
ch_regs-apb_ptr = tdc-dma_sconfig.src_addr;
ch_regs-apb_seq = get_bus_width(tdc,
tdc-dma_sconfig.src_addr_width);
ch_regs-csr = 0;
- return 0;
+ break;
default:
dev_err(tdc2dev(tdc), Dma direction is not supported\n);
return -EINVAL;
}
- return -EINVAL;
-}
-
-static int tegra_dma_get_xfer_params_sg(struct tegra_dma_channel *tdc,
- struct tegra_dma_sg_req *sg_req,
- enum dma_transfer_direction direction,
- unsigned int flags)
-{
- struct tegra_dma_channel_regs *ch_regs = sg_req-ch_regs;
- int ret;
-
- ret = tegra_dma_get_xfer_params(tdc, ch_regs, direction);
- if (ret 0)
- return ret;
+ ch_regs-apb_seq |= TEGRA_APBDMA_APBSEQ_WRAP_WORD_1;
ch_regs-ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
- ch_regs-ahb_seq |= TEGRA_APBDMA_AHBSEQ_WRAP_NONE
- TEGRA_APBDMA_AHBSEQ_WRAP_SHIFT;
ch_regs-ahb_seq |= TEGRA_APBDMA_AHBSEQ_BUS_WIDTH_32;
- ch_regs-csr |= TEGRA_APBDMA_CSR_ONCE | TEGRA_APBDMA_CSR_FLOW;
+ ch_regs-csr |= TEGRA_APBDMA_CSR_FLOW;
ch_regs-csr |= tdc-slave_id TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
+
if (flags DMA_PREP_INTERRUPT)
ch_regs-csr |= TEGRA_APBDMA_CSR_IE_EOC;
- ch_regs-apb_seq |= TEGRA_APBDMA_APBSEQ_WRAP_WORD_1;
-
return 0;
}
-static int tegra_dma_get_xfer_params_cyclic(struct tegra_dma_channel *tdc,
+static int tegra_dma_get_xfer_params_sg(struct tegra_dma_channel *tdc,
struct tegra_dma_sg_req *sg_req,
enum dma_transfer_direction direction,
unsigned int flags)
@@ -997,23 +982,23 @@ static int tegra_dma_get_xfer_params_cyclic(struct
tegra_dma_channel *tdc,
struct tegra_dma_channel_regs *ch_regs = sg_req-ch_regs;
int ret;
- ret = tegra_dma_get_xfer_params(tdc, ch_regs, direction);
+ ret = tegra_dma_get_xfer_params(tdc, ch_regs, direction, flags);
if (ret 0)
return ret;
- ch_regs-ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
- ch_regs-ahb_seq |= TEGRA_APBDMA_AHBSEQ_WRAP_NONE
- TEGRA_APBDMA_AHBSEQ_WRAP_SHIFT;
- ch_regs-ahb_seq |= TEGRA_APBDMA_AHBSEQ_BUS_WIDTH_32;
+ ch_regs-csr |= TEGRA_APBDMA_CSR_ONCE;
- ch_regs-csr |= TEGRA_APBDMA_CSR_FLOW;
- if (flags DMA_PREP_INTERRUPT)
- ch_regs-csr |= TEGRA_APBDMA_CSR_IE_EOC;
- ch_regs-csr |= tdc-slave_id TEGRA_APBDMA_CSR_REQ_SEL_SHIFT;
+ return 0;
+}
- ch_regs-apb_seq |= TEGRA_APBDMA_APBSEQ_WRAP_WORD_1;
+static int tegra_dma_get_xfer_params_cyclic(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req,
+ enum dma_transfer_direction direction,
+ unsigned int flags)
+{
+ struct tegra_dma_channel_regs *ch_regs = sg_req-ch_regs;
- return 0;
+ return tegra_dma_get_xfer_params(tdc, ch_regs, direction, flags);
}
static void tegra_dma_prep_wcount(struct
[RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA
Add support for the Tegra210 Audio DMA controller that is used for
transferring data between system memory and the Audio sub-system.
This driver is based upon the work by Dara Ramesh dram...@nvidia.com.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/dma/Kconfig | 12 +
drivers/dma/Makefile | 1 +
drivers/dma/tegra-common.c| 3 +-
drivers/dma/tegra-common.h| 42 ++-
drivers/dma/tegra20-apb-dma.c | 37 +--
drivers/dma/tegra210-adma.c | 710 ++
6 files changed, 782 insertions(+), 23 deletions(-)
create mode 100644 drivers/dma/tegra210-adma.c
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index dd79b0bf0876..25b474965d66 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -205,6 +205,18 @@ config TEGRA20_APB_DMA
This DMA controller transfers data from memory to peripheral fifo
or vice versa. It does not support memory to memory data transfer.
+config TEGRA210_ADMA
+ bool NVIDIA Tegra210 ADMA support
+ depends on ARCH_TEGRA
+ select DMA_ENGINE
+ select TEGRA_DMA_COMMON
+ help
+ Support for the NVIDIA Tegra210 ADMA controller driver. The
+ DMA controller is having multiple DMA channel and it configured
+ for audio. This DMA controller transfers data from memory to
+ peripheral fifo or vice versa. It does not support memory to
+ memory data transfer.
+
config S3C24XX_DMAC
tristate Samsung S3C24XX DMA support
depends on ARCH_S3C24XX
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index d9c2bf5ef0bd..9c5b8afc53a1 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
obj-$(CONFIG_TI_EDMA) += edma.o
obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
obj-$(CONFIG_TEGRA20_APB_DMA) += tegra20-apb-dma.o
+obj-$(CONFIG_TEGRA210_ADMA) += tegra210-adma.o
obj-$(CONFIG_TEGRA_DMA_COMMON) += tegra-common.o
obj-$(CONFIG_S3C24XX_DMAC) += s3c24xx-dma.o
obj-$(CONFIG_PL330_DMA) += pl330.o
diff --git a/drivers/dma/tegra-common.c b/drivers/dma/tegra-common.c
index fff0a143f5bb..b3f7e3322c15 100644
--- a/drivers/dma/tegra-common.c
+++ b/drivers/dma/tegra-common.c
@@ -620,7 +620,8 @@ struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
return NULL;
}
- if (ops-get_xfer_params_cyclic(tdc, sg_base, direction, flags) 0)
+ if (ops-get_xfer_params_cyclic(tdc, sg_base, buf_len, period_len,
+ direction, flags) 0)
return NULL;
dma_desc = tegra_dma_desc_get(tdc);
diff --git a/drivers/dma/tegra-common.h b/drivers/dma/tegra-common.h
index e0d4d2b13cb8..c1a369e7efa6 100644
--- a/drivers/dma/tegra-common.h
+++ b/drivers/dma/tegra-common.h
@@ -34,7 +34,7 @@ struct tegra_dma_chip_data {
/*
* DMA channel registers
*/
-struct tegra_dma_channel_regs {
+struct tegra_apb_chan_regs {
unsigned long csr;
unsigned long ahb_ptr;
unsigned long apb_ptr;
@@ -44,6 +44,18 @@ struct tegra_dma_channel_regs {
};
/*
+ * ADMA channel registers
+ */
+struct tegra_adma_chan_regs {
+ unsigned long ctrl;
+ unsigned long config;
+ unsigned long src_ptr;
+ unsigned long tgt_ptr;
+ unsigned long ahub_fifo_ctrl;
+ unsigned long tc;
+};
+
+/*
* tegra_dma_sg_req: Dma request details to configure hardware. This
* contains the details for one transfer to configure DMA hw.
* The client's request for data transfer can be broken into multiple
@@ -52,7 +64,10 @@ struct tegra_dma_channel_regs {
* DMA descriptor which manages the transfer details.
*/
struct tegra_dma_sg_req {
- struct tegra_dma_channel_regs ch_regs;
+ union {
+ struct tegra_apb_chan_regs apb_ch_regs;
+ struct tegra_adma_chan_regs adma_ch_regs;
+ };
int req_len;
boolconfigured;
boollast_sg;
@@ -109,7 +124,10 @@ struct tegra_dma_channel {
/* Channel-slave specific configuration */
unsigned int slave_id;
struct dma_slave_config dma_sconfig;
- struct tegra_dma_channel_regs channel_reg;
+ union {
+ struct tegra_apb_chan_regs apb_ch_regs;
+ struct tegra_adma_chan_regs adma_ch_regs;
+ };
};
/*
@@ -119,6 +137,7 @@ struct tegra_dma_ops {
u32 (*get_xfer_count)(struct tegra_dma_channel *tdc);
int (*get_xfer_params_cyclic)(struct tegra_dma_channel *tdc,
struct tegra_dma_sg_req *sg_req,
+ size_t buf_len, size_t period_len,
enum dma_transfer_direction direction,
unsigned int flags);
int (*get_xfer_params_sg)(struct
[RFC PATCH 0/7] DMA: Add support for Tegra210 ADMA
Add support for Tegra210 Audio DMA (ADMA) controller. This driver is based upon the existing Tegra20-APB DMA driver and a lot of the core code has been re-used. This is currently being distributed as an RFC to get feedback on the approach and although this has been compile and boot tested, it still needs further functional testing to ensure it is working well. This series is based upon the Tegra-APB clean-up series [0]. [0] https://lkml.org/lkml/2015/8/6/315 Jon Hunter (7): DMA: tegra-apb: Correct runtime-pm usage DMA: tegra-apb: Move code dealing with h/w registers into separate functions DMA: tegra-apb: Clean-up and simplify setting up of transfer parameters DMA: tegra-apb: Add a function table for functions dealing with registers DMA: tegra-apb: Move common code into separate source files Documentation: DT: Add binding documentation for NVIDIA ADMA DMA: tegra-adma: Add support for Tegra210 ADMA .../devicetree/bindings/dma/tegra210-adma.txt | 49 + drivers/dma/Kconfig| 16 + drivers/dma/Makefile |2 + drivers/dma/tegra-common.c | 734 + drivers/dma/tegra-common.h | 260 + drivers/dma/tegra20-apb-dma.c | 1121 +++- drivers/dma/tegra210-adma.c| 710 + 7 files changed, 1928 insertions(+), 964 deletions(-) create mode 100644 Documentation/devicetree/bindings/dma/tegra210-adma.txt create mode 100644 drivers/dma/tegra-common.c create mode 100644 drivers/dma/tegra-common.h create mode 100644 drivers/dma/tegra210-adma.c -- 2.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 1/7] DMA: tegra-apb: Correct runtime-pm usage
The tegra-apb DMA driver enables runtime-pm but never calls
pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
The driver manages the clocks by directly calling clk_prepare_enable()
and clk_unprepare_disable().
Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
the consequence of this is that if runtime-pm is disabled, then the clocks
will remain on the entire time the driver is loaded. However, if
runtime-pm is disabled, then power is not most likely not a concern.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/dma/tegra20-apb-dma.c | 36
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c8f79dcaaee8..097432ea89fa 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct
dma_chan *dc)
{
struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
struct tegra_dma *tdma = tdc-tdma;
- int ret;
dma_cookie_init(tdc-dma_chan);
tdc-config_init = false;
- ret = clk_prepare_enable(tdma-dma_clk);
- if (ret 0)
- dev_err(tdc2dev(tdc), clk_prepare_enable failed: %d\n, ret);
- return ret;
+
+ return pm_runtime_get_sync(tdma-dev);
}
static void tegra_dma_free_chan_resources(struct dma_chan *dc)
@@ -1232,7 +1229,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan
*dc)
list_del(sg_req-node);
kfree(sg_req);
}
- clk_disable_unprepare(tdma-dma_clk);
+ pm_runtime_put(tdma-dev);
tdc-slave_id = 0;
}
@@ -1356,21 +1353,13 @@ static int tegra_dma_probe(struct platform_device *pdev)
spin_lock_init(tdma-global_lock);
pm_runtime_enable(pdev-dev);
- if (!pm_runtime_enabled(pdev-dev)) {
+ if (!pm_runtime_enabled(pdev-dev))
ret = tegra_dma_runtime_resume(pdev-dev);
- if (ret) {
- dev_err(pdev-dev, dma_runtime_resume failed %d\n,
- ret);
- goto err_pm_disable;
- }
- }
+ else
+ ret = pm_runtime_get_sync(pdev-dev);
- /* Enable clock before accessing registers */
- ret = clk_prepare_enable(tdma-dma_clk);
- if (ret 0) {
- dev_err(pdev-dev, clk_prepare_enable failed: %d\n, ret);
+ if (ret)
goto err_pm_disable;
- }
/* Reset DMA controller */
reset_control_assert(tdma-rst);
@@ -1382,7 +1371,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xul);
- clk_disable_unprepare(tdma-dma_clk);
+ pm_runtime_put(pdev-dev);
INIT_LIST_HEAD(tdma-dma_dev.channels);
for (i = 0; i cdata-nr_channels; i++) {
@@ -1484,7 +1473,6 @@ err_irq:
struct tegra_dma_channel *tdc = tdma-channels[i];
tasklet_kill(tdc-tasklet);
}
-
err_pm_disable:
pm_runtime_disable(pdev-dev);
if (!pm_runtime_status_suspended(pdev-dev))
@@ -1543,7 +1531,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
int ret;
/* Enable clock before accessing register */
- ret = tegra_dma_runtime_resume(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret 0)
return ret;
@@ -1560,7 +1548,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
}
/* Disable clock */
- tegra_dma_runtime_suspend(dev);
+ pm_runtime_put(dev);
return 0;
}
@@ -1571,7 +1559,7 @@ static int tegra_dma_pm_resume(struct device *dev)
int ret;
/* Enable clock before accessing register */
- ret = tegra_dma_runtime_resume(dev);
+ ret = pm_runtime_get_sync(dev);
if (ret 0)
return ret;
@@ -1592,7 +1580,7 @@ static int tegra_dma_pm_resume(struct device *dev)
}
/* Disable clock */
- tegra_dma_runtime_suspend(dev);
+ pm_runtime_put(dev);
return 0;
}
#endif
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RFC PATCH 4/7] DMA: tegra-apb: Add a function table for functions dealing with registers
In preparation for adding the Tegra210 ADMA driver, add a function table
for calling functions that access hardware registers. This way code that
is common between the Tegra20-APB DMA and Tegra210 DMA driver can be moved
into a separate source file and used by both DMA drivers.
Note that all function pointers in the table are compulsory and so no
checking that the function pointer is valid is performed.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/dma/tegra20-apb-dma.c | 92 +--
1 file changed, 71 insertions(+), 21 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c1eb25075756..7947acdf23db 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -209,6 +209,33 @@ struct tegra_dma_channel {
struct tegra_dma_channel_regs channel_reg;
};
+struct tegra_dma_ops {
+ u32 (*get_xfer_count)(struct tegra_dma_channel *tdc);
+ int (*get_xfer_params_cyclic)(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req,
+ enum dma_transfer_direction direction,
+ unsigned int flags);
+ int (*get_xfer_params_sg)(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req,
+ enum dma_transfer_direction direction,
+ unsigned int flags);
+ u32 (*irq_clear)(struct tegra_dma_channel *tdc);
+ u32 (*irq_status)(struct tegra_dma_channel *tdc);
+ void (*pause)(struct tegra_dma_channel *tdc,
+ bool wait_for_burst_complete);
+ void (*program)(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req);
+ void (*resume)(struct tegra_dma_channel *tdc);
+ void (*set_xfer_params)(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req,
+ struct tegra_dma_sg_req *sg_base,
+ enum dma_transfer_direction direction,
+ u32 mem, u32 len);
+ void (*start)(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *sg_req);
+ void (*stop)(struct tegra_dma_channel *tdc);
+};
+
/* tegra_dma: Tegra DMA specific information */
struct tegra_dma {
struct dma_device dma_dev;
@@ -218,6 +245,7 @@ struct tegra_dma {
spinlock_t global_lock;
void __iomem*base_addr;
const struct tegra_dma_chip_data *chip_data;
+ const struct tegra_dma_ops *ops;
/*
* Counter for managing global pausing of the DMA controller.
@@ -504,6 +532,7 @@ static void tegra_dma_start(struct tegra_dma_channel *tdc,
static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
struct tegra_dma_sg_req *nsg_req)
{
+ const struct tegra_dma_ops *ops = tdc-tdma-ops;
unsigned long status;
/*
@@ -517,8 +546,8 @@ static void tegra_dma_configure_for_next(struct
tegra_dma_channel *tdc,
* If there is already IEC status then interrupt handler need to
* load new configuration.
*/
- tegra_dma_pause(tdc, false);
- status = tegra_dma_irq_status(tdc);
+ ops-pause(tdc, false);
+ status = ops-irq_status(tdc);
/*
* If interrupt is pending then do nothing as the ISR will handle
@@ -527,17 +556,18 @@ static void tegra_dma_configure_for_next(struct
tegra_dma_channel *tdc,
if (status) {
dev_err(tdc2dev(tdc),
Skipping new configuration as interrupt is pending\n);
- tegra_dma_resume(tdc);
+ ops-resume(tdc);
return;
}
/* Safe to program new configuration */
- tegra_dma_program(tdc, nsg_req);
- tegra_dma_resume(tdc);
+ ops-program(tdc, nsg_req);
+ ops-resume(tdc);
}
static void tdc_start_head_req(struct tegra_dma_channel *tdc)
{
+ const struct tegra_dma_ops *ops = tdc-tdma-ops;
struct tegra_dma_sg_req *sg_req;
if (list_empty(tdc-pending_sg_req))
@@ -545,7 +575,7 @@ static void tdc_start_head_req(struct tegra_dma_channel
*tdc)
sg_req = list_first_entry(tdc-pending_sg_req,
typeof(*sg_req), node);
- tegra_dma_start(tdc, sg_req);
+ ops-start(tdc, sg_req);
sg_req-configured = true;
tdc-busy = true;
}
@@ -599,11 +629,12 @@ static void tegra_dma_abort_all(struct tegra_dma_channel
*tdc)
static bool handle_continuous_head_request(struct tegra_dma_channel *tdc,
struct tegra_dma_sg_req *last_sg_req, bool to_terminate)
{
+ const struct tegra_dma_ops *ops = tdc-tdma-ops;
struct tegra_dma_sg_req *hsgreq = NULL
[RFC PATCH 2/7] DMA: tegra-apb: Move code dealing with h/w registers into separate functions
In preparation for adding the Tegra210 ADMA driver, that is based upon the
Tegra20-APB DMA driver, move code that accesses hardware registers into
specific functions. The Tegra210 ADMA and Tegra20-APB DMA drivers are not
compatible from a hardware register perspective, but the drivers are very
much the same. Hence, by isolating code that deals with the hardware
registers it will then be possible to add a function table to call code
that accesses the hardware registers and re-use the common driver code
for both DMAs.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/dma/tegra20-apb-dma.c | 277 ++
1 file changed, 170 insertions(+), 107 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 097432ea89fa..e552a4efef71 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -359,6 +359,18 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
return 0;
}
+static u32 tegra_dma_get_xfer_count(struct tegra_dma_channel *tdc)
+{
+ u32 wcount;
+
+ if (tdc-tdma-chip_data-support_separate_wcount_reg)
+ wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
+ else
+ wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+
+ return (wcount TEGRA_APBDMA_STATUS_COUNT_MASK) - 4;
+}
+
static void tegra_dma_global_pause(struct tegra_dma_channel *tdc,
bool wait_for_burst_complete)
{
@@ -394,6 +406,38 @@ out:
spin_unlock(tdma-global_lock);
}
+static u32 tegra_dma_irq_status(struct tegra_dma_channel *tdc)
+{
+ u32 status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+
+ return status TEGRA_APBDMA_STATUS_ISE_EOC;
+}
+
+static u32 tegra_dma_irq_clear(struct tegra_dma_channel *tdc)
+{
+ u32 status = tegra_dma_irq_status(tdc);
+
+ if (status) {
+ dev_dbg(tdc2dev(tdc), %s():clearing interrupt\n, __func__);
+ tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
+ }
+
+ return status;
+}
+
+static void tegra_dma_program(struct tegra_dma_channel *tdc,
+ struct tegra_dma_sg_req *nsg_req)
+{
+ tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, nsg_req-ch_regs.apb_ptr);
+ tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, nsg_req-ch_regs.ahb_ptr);
+ if (tdc-tdma-chip_data-support_separate_wcount_reg)
+ tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
+ nsg_req-ch_regs.wcount);
+ tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
+ nsg_req-ch_regs.csr | TEGRA_APBDMA_CSR_ENB);
+ nsg_req-configured = true;
+}
+
static void tegra_dma_pause(struct tegra_dma_channel *tdc,
bool wait_for_burst_complete)
{
@@ -423,7 +467,6 @@ static void tegra_dma_resume(struct tegra_dma_channel *tdc)
static void tegra_dma_stop(struct tegra_dma_channel *tdc)
{
u32 csr;
- u32 status;
/* Disable interrupts */
csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
@@ -435,11 +478,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR, csr);
/* Clear interrupt status if it is there */
- status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
- if (status TEGRA_APBDMA_STATUS_ISE_EOC) {
- dev_dbg(tdc2dev(tdc), %s():clearing interrupt\n, __func__);
- tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
- }
+ tegra_dma_irq_clear(tdc);
+
tdc-busy = false;
}
@@ -478,13 +518,13 @@ static void tegra_dma_configure_for_next(struct
tegra_dma_channel *tdc,
* load new configuration.
*/
tegra_dma_pause(tdc, false);
- status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+ status = tegra_dma_irq_status(tdc);
/*
* If interrupt is pending then do nothing as the ISR will handle
* the programing for new request.
*/
- if (status TEGRA_APBDMA_STATUS_ISE_EOC) {
+ if (status) {
dev_err(tdc2dev(tdc),
Skipping new configuration as interrupt is pending\n);
tegra_dma_resume(tdc);
@@ -492,15 +532,7 @@ static void tegra_dma_configure_for_next(struct
tegra_dma_channel *tdc,
}
/* Safe to program new configuration */
- tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, nsg_req-ch_regs.apb_ptr);
- tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, nsg_req-ch_regs.ahb_ptr);
- if (tdc-tdma-chip_data-support_separate_wcount_reg)
- tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
- nsg_req-ch_regs.wcount);
- tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
- nsg_req-ch_regs.csr | TEGRA_APBDMA_CSR_ENB);
- nsg_req-configured = true;
-
+ tegra_dma_program(tdc, nsg_req);
tegra_dma_resume(tdc);
}
@@ -534,10 +566,10 @@ static void tdc_configure_next_head_desc(struct
Re: [RFC PATCH 1/7] DMA: tegra-apb: Correct runtime-pm usage
On 23/08/15 15:17, Vinod Koul wrote: On Tue, Aug 18, 2015 at 02:49:09PM +0100, Jon Hunter wrote: @@ -1543,7 +1531,7 @@ static int tegra_dma_pm_suspend(struct device *dev) int ret; /* Enable clock before accessing register */ -ret = tegra_dma_runtime_resume(dev); +ret = pm_runtime_get_sync(dev); why is this required ? Because the clock could be disabled when this function is called. This function saves the DMA context so that if the context is lost during suspend, it can be restored. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 7/7] DMA: tegra-adma: Add support for Tegra210 ADMA
On 23/08/15 15:33, Vinod Koul wrote:
On Tue, Aug 18, 2015 at 02:49:15PM +0100, Jon Hunter wrote:
+#define AHUB_TO_MEMORY 2
+#define MEMORY_TO_AHUB 4
namespace this aptly as well
+static void tegra_adma_stop(struct tegra_dma_channel *tdc)
+{
+u32 status;
+
+/* TODO: Do we need to disable interrupts here? */
when?
Once everyone is happy with the RFC in general.
+static void tegra_adma_start(struct tegra_dma_channel *tdc,
+struct tegra_dma_sg_req *sg_req)
+{
+struct tegra_adma_chan_regs *ch_regs = sg_req-adma_ch_regs;
+
+/* Update transfer done count for position calculation */
+tdc-adma_ch_regs.tc = ch_regs-tc;
+tdc_write(tdc, ADMA_CH_TC, ch_regs-tc);
+tdc_write(tdc, ADMA_CH_CTRL, ch_regs-ctrl);
+tdc_write(tdc, ADMA_CH_LOWER_SOURCE_ADDR, ch_regs-src_ptr);
+tdc_write(tdc, ADMA_CH_LOWER_TARGET_ADDR, ch_regs-tgt_ptr);
+tdc_write(tdc, ADMA_CH_AHUB_FIFO_CTRL, ch_regs-ahub_fifo_ctrl);
+tdc_write(tdc, ADMA_CH_CONFIG, ch_regs-config);
empty line here please
Ok.
+static int tegra_adma_get_xfer_params(struct tegra_dma_channel *tdc,
+ struct tegra_adma_chan_regs *ch_regs,
+ enum dma_transfer_direction direction)
+{
+u32 burst_size, ctrl, ctrl_mask, slave_id, fifo_mask, fifo_shift;
+
+ch_regs-ahub_fifo_ctrl = tdc_read(tdc, ADMA_CH_AHUB_FIFO_CTRL);
+ch_regs-config = tdc_read(tdc, ADMA_CH_CONFIG);
+ch_regs-ctrl = tdc_read(tdc, ADMA_CH_CTRL);
+slave_id = tdc-dma_sconfig.slave_id;
+
+switch (direction) {
+case DMA_MEM_TO_DEV:
+burst_size = fls(tdc-dma_sconfig.dst_maxburst);
+ctrl_mask = ADMA_CH_CTRL_TX_REQUEST_SELECT_MASK;
+ctrl = MEMORY_TO_AHUB ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
+ctrl |= slave_id ADMA_CH_CTRL_TX_REQUEST_SELECT_SHIFT;
+fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_MASK;
+fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_TX_FIFO_SIZE_SHIFT;
+break;
Empty line here pls
Ok, any reason why? Other dma drivers don't appear to do this.
+case DMA_DEV_TO_MEM:
+burst_size = fls(tdc-dma_sconfig.src_maxburst);
+ctrl_mask = ADMA_CH_CTRL_RX_REQUEST_SELECT_MASK;
+ctrl = AHUB_TO_MEMORY ADMA_CH_CTRL_TRANSFER_DIRECTION_SHIFT;
+ctrl |= slave_id ADMA_CH_CTRL_RX_REQUEST_SELECT_SHIFT;
+fifo_mask = ADMA_CH_AHUB_FIFO_CTRL_RX_FIFO_SIZE_MASK;
+fifo_shift = ADMA_CH_AHUB_FIFO_CTRL_RX_FIFO_SIZE_SHIFT;
+break;
here too...
+default:
+dev_err(tdc2dev(tdc), Dma direction is not supported\n);
+return -EINVAL;
+}
+
+if (!burst_size || burst_size ADMA_BURSTSIZE_16)
+burst_size = ADMA_BURSTSIZE_16;
+
+ch_regs-ahub_fifo_ctrl = ~fifo_mask;
+ch_regs-ahub_fifo_ctrl |= ADMA_FIFO_DEFAULT_SIZE fifo_shift;
+ch_regs-config = ~ADMA_CH_CONFIG_BURST_SIZE_MASK;
+ch_regs-config |= burst_size ADMA_CH_CONFIG_BURST_SIZE_SHIFT;
+ch_regs-ctrl = ~(ctrl_mask | ADMA_CH_CTRL_TRANSFER_DIRECTION_MASK);
+ch_regs-ctrl |= ctrl;
+
+return -EINVAL;
??
Thanks. That's an error. Will fix.
+static int tegra_adma_pm_suspend(struct device *dev)
+{
+struct tegra_dma *tdma = dev_get_drvdata(dev);
+int i;
+int ret;
+
+ret = pm_runtime_get_sync(dev);
why is this required :)
To ensure that the clocks are enabled before the registers are read.
This function saves the dma context before suspend, in case the hardware
state is lost.
+static int tegra_adma_pm_resume(struct device *dev)
+{
+struct tegra_dma *tdma = dev_get_drvdata(dev);
+int i;
+int ret;
+
+ret = pm_runtime_get_sync(dev);
and this
Same here.
Cheers
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: gic: Add a cpu map for each GIC instance
On 29/07/15 19:33, Russell King - ARM Linux wrote: On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote: The gic_init_bases() function initialises an array that stores the mapping between the GIC and CPUs. This array is a global array that is unconditionally initialised on every call to gic_init_bases(). Although, it is not common for there to be more than one GIC instance, there are some devices that do support nested GIC controllers and gic_init_bases() can be called more than once. A 2nd call to gic_init_bases() will clear the previous CPU mapping and will only setup the mapping again for CPU0. This is because for child GIC controllers there is most likely only one recipient of the interrupt. Fix this by moving the CPU mapping array to the GIC chip data structure so that it is initialised for each GIC instance separately. It is assumed that the bL switcher code is only interested in the root or primary GIC instance. Does it make sense to expose the per-CPU-ness of the non-primary GIC? If they are chained off a primary GIC SPI interrupt, then all IRQs on the secondary GIC are routed to the same CPU that the SPI on the primary GIC is routed to. I am looking at a use-case where there is a secondary GIC and the secondary GIC is used as a interrupt router between the main CPU cluster and another CPU. So in this case the mapping of a secondary is still of interest. This patch does not address setting up the secondary mapping, but avoids a secondary GIC overwriting the primary map (which we don't want). Other features like the PPIs and SGIs in the secondary CPU should also be ignored - they probably aren't used anyway. Yes, agree. I have to say though... are the 1020 IRQs that the primary GIC provides really not enough? What insane hardware needs more than 1020 IRQs? Ha. I guess some realview boards for a start ... # grep -r gic_init(1 arch/arm/ arch/arm/mach-realview/realview_pb1176.c: gic_init(1, IRQ_PB1176_GIC_START, arch/arm/mach-realview/realview_eb.c: gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE), arch/arm/mach-realview/realview_pb11mp.c: gic_init(1, IRQ_PB11MP_GIC_START, Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] irqchip/gic: Only allow the primary GIC to set the CPU map
The gic_init_bases() function initialises an array that stores the mapping
between the GIC and CPUs. This array is a global array that is
unconditionally initialised on every call to gic_init_bases(). Although,
it is not common for there to be more than one GIC instance, there are
some devices that do support nested GIC controllers and gic_init_bases()
can be called more than once.
A 2nd call to gic_init_bases() will clear the previous CPU mapping and
will only setup the mapping again for the CPU calling gic_init_bases().
Fix this by only allowing the CPU map to be configured for the primary GIC.
For secondary GICs the CPU map is not relevant because these GICs do not
directly route the interrupts to the main CPU(s) but to other GICs or
devices.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
This is a follow-up to the patch titled irqchip: gic: Add a cpu map for
each GIC instance and discussed here [1]. Based upon the discussion I have
re-worked and re-titled it approriately.
[1] http://www.spinics.net/lists/kernel/msg2044421.html
drivers/irqchip/irq-gic.c | 43 +--
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a530d9a9b810..7566fe259d27 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -416,19 +416,26 @@ static void gic_cpu_init(struct gic_chip_data *gic)
int i;
/*
-* Get what the GIC says our CPU mask is.
+* Setting up the CPU map is only relevant for the primary GIC
+* because any nested/secondary GICs do not directly interface
+* with the CPU(s).
*/
- BUG_ON(cpu = NR_GIC_CPU_IF);
- cpu_mask = gic_get_cpumask(gic);
- gic_cpu_map[cpu] = cpu_mask;
+ if (gic == gic_data[0]) {
+ /*
+* Get what the GIC says our CPU mask is.
+*/
+ BUG_ON(cpu = NR_GIC_CPU_IF);
+ cpu_mask = gic_get_cpumask(gic);
+ gic_cpu_map[cpu] = cpu_mask;
- /*
-* Clear our mask from the other map entries in case they're
-* still undefined.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- if (i != cpu)
- gic_cpu_map[i] = ~cpu_mask;
+ /*
+* Clear our mask from the other map entries in case they're
+* still undefined.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ if (i != cpu)
+ gic_cpu_map[i] = ~cpu_mask;
+ }
gic_cpu_config(dist_base, NULL);
@@ -977,13 +984,6 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
}
/*
-* Initialize the CPU interface map to all CPUs.
-* It will be refined as each CPU probes its ID.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- gic_cpu_map[i] = 0xff;
-
- /*
* Find out how many interrupts are supported.
* The GIC only supports up to 1020 interrupt sources.
*/
@@ -1028,6 +1028,13 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
return;
if (gic_nr == 0) {
+ /*
+* Initialize the CPU interface map to all CPUs.
+* It will be refined as each CPU probes its ID.
+* This is only necessary for the primary GIC.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ gic_cpu_map[i] = 0xff;
#ifdef CONFIG_SMP
set_smp_cross_call(gic_raise_softirq);
register_cpu_notifier(gic_cpu_notifier);
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip/gic: Only allow the primary GIC to set the CPU map
On 30/07/15 15:33, Marc Zyngier wrote:
On 30/07/15 15:11, Jon Hunter wrote:
The gic_init_bases() function initialises an array that stores the mapping
between the GIC and CPUs. This array is a global array that is
unconditionally initialised on every call to gic_init_bases(). Although,
it is not common for there to be more than one GIC instance, there are
some devices that do support nested GIC controllers and gic_init_bases()
can be called more than once.
A 2nd call to gic_init_bases() will clear the previous CPU mapping and
will only setup the mapping again for the CPU calling gic_init_bases().
Fix this by only allowing the CPU map to be configured for the primary GIC.
For secondary GICs the CPU map is not relevant because these GICs do not
directly route the interrupts to the main CPU(s) but to other GICs or
devices.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
This is a follow-up to the patch titled irqchip: gic: Add a cpu map for
each GIC instance and discussed here [1]. Based upon the discussion I have
re-worked and re-titled it approriately.
[1] http://www.spinics.net/lists/kernel/msg2044421.html
drivers/irqchip/irq-gic.c | 43 +--
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a530d9a9b810..7566fe259d27 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -416,19 +416,26 @@ static void gic_cpu_init(struct gic_chip_data *gic)
int i;
/*
- * Get what the GIC says our CPU mask is.
+ * Setting up the CPU map is only relevant for the primary GIC
+ * because any nested/secondary GICs do not directly interface
+ * with the CPU(s).
*/
-BUG_ON(cpu = NR_GIC_CPU_IF);
-cpu_mask = gic_get_cpumask(gic);
-gic_cpu_map[cpu] = cpu_mask;
+if (gic == gic_data[0]) {
+/*
+ * Get what the GIC says our CPU mask is.
+ */
+BUG_ON(cpu = NR_GIC_CPU_IF);
+cpu_mask = gic_get_cpumask(gic);
+gic_cpu_map[cpu] = cpu_mask;
-/*
- * Clear our mask from the other map entries in case they're
- * still undefined.
- */
-for (i = 0; i NR_GIC_CPU_IF; i++)
-if (i != cpu)
-gic_cpu_map[i] = ~cpu_mask;
+/*
+ * Clear our mask from the other map entries in case they're
+ * still undefined.
+ */
+for (i = 0; i NR_GIC_CPU_IF; i++)
+if (i != cpu)
+gic_cpu_map[i] = ~cpu_mask;
+}
gic_cpu_config(dist_base, NULL);
@@ -977,13 +984,6 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
}
/*
- * Initialize the CPU interface map to all CPUs.
- * It will be refined as each CPU probes its ID.
- */
-for (i = 0; i NR_GIC_CPU_IF; i++)
-gic_cpu_map[i] = 0xff;
-
-/*
* Find out how many interrupts are supported.
* The GIC only supports up to 1020 interrupt sources.
*/
@@ -1028,6 +1028,13 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
return;
if (gic_nr == 0) {
+/*
+ * Initialize the CPU interface map to all CPUs.
+ * It will be refined as each CPU probes its ID.
+ * This is only necessary for the primary GIC.
+ */
+for (i = 0; i NR_GIC_CPU_IF; i++)
+gic_cpu_map[i] = 0xff;
#ifdef CONFIG_SMP
set_smp_cross_call(gic_raise_softirq);
register_cpu_notifier(gic_cpu_notifier);
Looks good.
I think there is a another bug caused by 322895062 (irqchip: gic:
Preserve gic V2 bypass bits in cpu ctrl register), where
gic_cpu_if_up() only acts on the primary GIC. In the secondary GIC case,
it will stay disabled.
Mind fixing this while you're at it?
Ah yes, I see. Ok, I will resend this with the other fix as a series.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip/gic: Only allow the primary GIC to set the CPU map
On 30/07/15 16:05, Jon Hunter wrote:
On 30/07/15 15:33, Marc Zyngier wrote:
On 30/07/15 15:11, Jon Hunter wrote:
The gic_init_bases() function initialises an array that stores the mapping
between the GIC and CPUs. This array is a global array that is
unconditionally initialised on every call to gic_init_bases(). Although,
it is not common for there to be more than one GIC instance, there are
some devices that do support nested GIC controllers and gic_init_bases()
can be called more than once.
A 2nd call to gic_init_bases() will clear the previous CPU mapping and
will only setup the mapping again for the CPU calling gic_init_bases().
Fix this by only allowing the CPU map to be configured for the primary GIC.
For secondary GICs the CPU map is not relevant because these GICs do not
directly route the interrupts to the main CPU(s) but to other GICs or
devices.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
This is a follow-up to the patch titled irqchip: gic: Add a cpu map for
each GIC instance and discussed here [1]. Based upon the discussion I have
re-worked and re-titled it approriately.
[1] http://www.spinics.net/lists/kernel/msg2044421.html
drivers/irqchip/irq-gic.c | 43 +--
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a530d9a9b810..7566fe259d27 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -416,19 +416,26 @@ static void gic_cpu_init(struct gic_chip_data *gic)
int i;
/*
-* Get what the GIC says our CPU mask is.
+* Setting up the CPU map is only relevant for the primary GIC
+* because any nested/secondary GICs do not directly interface
+* with the CPU(s).
*/
- BUG_ON(cpu = NR_GIC_CPU_IF);
- cpu_mask = gic_get_cpumask(gic);
- gic_cpu_map[cpu] = cpu_mask;
+ if (gic == gic_data[0]) {
+ /*
+* Get what the GIC says our CPU mask is.
+*/
+ BUG_ON(cpu = NR_GIC_CPU_IF);
+ cpu_mask = gic_get_cpumask(gic);
+ gic_cpu_map[cpu] = cpu_mask;
- /*
-* Clear our mask from the other map entries in case they're
-* still undefined.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- if (i != cpu)
- gic_cpu_map[i] = ~cpu_mask;
+ /*
+* Clear our mask from the other map entries in case they're
+* still undefined.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ if (i != cpu)
+ gic_cpu_map[i] = ~cpu_mask;
+ }
gic_cpu_config(dist_base, NULL);
@@ -977,13 +984,6 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
}
/*
-* Initialize the CPU interface map to all CPUs.
-* It will be refined as each CPU probes its ID.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- gic_cpu_map[i] = 0xff;
-
- /*
* Find out how many interrupts are supported.
* The GIC only supports up to 1020 interrupt sources.
*/
@@ -1028,6 +1028,13 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
return;
if (gic_nr == 0) {
+ /*
+* Initialize the CPU interface map to all CPUs.
+* It will be refined as each CPU probes its ID.
+* This is only necessary for the primary GIC.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ gic_cpu_map[i] = 0xff;
#ifdef CONFIG_SMP
set_smp_cross_call(gic_raise_softirq);
register_cpu_notifier(gic_cpu_notifier);
Looks good.
I think there is a another bug caused by 322895062 (irqchip: gic:
Preserve gic V2 bypass bits in cpu ctrl register), where
gic_cpu_if_up() only acts on the primary GIC. In the secondary GIC case,
it will stay disabled.
Mind fixing this while you're at it?
Ah yes, I see. Ok, I will resend this with the other fix as a series.
Hmmm, what about gic_cpu_if_down()? Looks like the only user is
vexpress-tc2. Ideally it should pass the gic_nr. I could make it pass 0
by default.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip/gic: Ensure gic_cpu_if_up/down() programs correct GIC instance
On 30/07/15 19:11, Marc Zyngier wrote:
On 30/07/15 18:56, Jon Hunter wrote:
On 30/07/15 17:51, Marc Zyngier wrote:
On 30/07/15 17:26, Jon Hunter wrote:
Commit 3228950621d9 (irqchip: gic: Preserve gic V2 bypass bits in cpu
ctrl register) added a new function, gic_cpu_if_up(), to program the
GIC CPU_CTRL register. This function assumes that there is only one GIC
instance present and hence always uses the chip data for the primary GIC
controller. Although it is not common for there to be a secondary, some
devices do support a secondary. Therefore, fix this by passing
gic_cpu_if_up() a pointer to the appropriate chip data structure.
Similarly, the function gic_cpu_if_down() only assumes that there is a
single GIC instance present. Update this function so that an instance
number is passed for the appropriate GIC. The vexpress TC2 (which has
a single GIC) is currently the only user of this function and so update
it accordingly.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
I was hoping to make the gic_cpu_if_up/down function more symmetric as we
discussed but it is not possible to pass the gic_nr to gic_cpu_if_up()
from all the places called without making more changes. However, given
that gic_cpu_if_up() is a local function and gic_cpu_if_down() is public,
may be it does not matter too much.
arch/arm/mach-vexpress/tc2_pm.c | 2 +-
drivers/irqchip/irq-gic.c | 14 +++---
include/linux/irqchip/arm-gic.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-vexpress/tc2_pm.c
b/arch/arm/mach-vexpress/tc2_pm.c
index b3328cd46c33..1aa4ccece69f 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -80,7 +80,7 @@ static void tc2_pm_cpu_powerdown_prepare(unsigned int
cpu, unsigned int cluster)
* to the CPU by disabling the GIC CPU IF to prevent wfi
* from completing execution behind power controller back
*/
- gic_cpu_if_down();
+ gic_cpu_if_down(0);
}
static void tc2_pm_cluster_powerdown_prepare(unsigned int cluster)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7566fe259d27..cf9aca22120f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -356,10 +356,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
return mask;
}
-static void gic_cpu_if_up(void)
+static void gic_cpu_if_up(struct gic_chip_data *gic)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
- void __iomem *dist_base = gic_data_dist_base(gic_data[0]);
+ void __iomem *cpu_base = gic_data_cpu_base(gic);
+ void __iomem *dist_base = gic_data_dist_base(gic);
Which tree is that against? I don't have a dist_base in mainline...
It is based upon linux-next. I can rebase on the current mainline if you
want them for v4.2.
It'd be good to fix it in mainline ASAP, as the Realview platforms are a
tiny bit dead at the moment, so a 4.2-rc would be good. The conflict
with Russell's FIQ work won't be hard to solve anyway.
Ok, will rebase on 4.2-rc4. I will also fix-up my incompetence, I just
pointed out in the above.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip/gic: Ensure gic_cpu_if_up/down() programs correct GIC instance
On 30/07/15 17:26, Jon Hunter wrote:
Commit 3228950621d9 (irqchip: gic: Preserve gic V2 bypass bits in cpu
ctrl register) added a new function, gic_cpu_if_up(), to program the
GIC CPU_CTRL register. This function assumes that there is only one GIC
instance present and hence always uses the chip data for the primary GIC
controller. Although it is not common for there to be a secondary, some
devices do support a secondary. Therefore, fix this by passing
gic_cpu_if_up() a pointer to the appropriate chip data structure.
Similarly, the function gic_cpu_if_down() only assumes that there is a
single GIC instance present. Update this function so that an instance
number is passed for the appropriate GIC. The vexpress TC2 (which has
a single GIC) is currently the only user of this function and so update
it accordingly.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
I was hoping to make the gic_cpu_if_up/down function more symmetric as we
discussed but it is not possible to pass the gic_nr to gic_cpu_if_up()
from all the places called without making more changes. However, given
that gic_cpu_if_up() is a local function and gic_cpu_if_down() is public,
may be it does not matter too much.
arch/arm/mach-vexpress/tc2_pm.c | 2 +-
drivers/irqchip/irq-gic.c | 14 +++---
include/linux/irqchip/arm-gic.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index b3328cd46c33..1aa4ccece69f 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -80,7 +80,7 @@ static void tc2_pm_cpu_powerdown_prepare(unsigned int cpu,
unsigned int cluster)
* to the CPU by disabling the GIC CPU IF to prevent wfi
* from completing execution behind power controller back
*/
- gic_cpu_if_down();
+ gic_cpu_if_down(0);
}
static void tc2_pm_cluster_powerdown_prepare(unsigned int cluster)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7566fe259d27..cf9aca22120f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -356,10 +356,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
return mask;
}
-static void gic_cpu_if_up(void)
+static void gic_cpu_if_up(struct gic_chip_data *gic)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
- void __iomem *dist_base = gic_data_dist_base(gic_data[0]);
+ void __iomem *cpu_base = gic_data_cpu_base(gic);
+ void __iomem *dist_base = gic_data_dist_base(gic);
u32 bypass = 0;
/*
@@ -451,12 +451,12 @@ static void gic_cpu_init(struct gic_chip_data *gic)
}
writel_relaxed(GICC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
- gic_cpu_if_up();
+ gic_cpu_if_up(gic);
}
-void gic_cpu_if_down(void)
+void gic_cpu_if_down(unsigned int gic_nr)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
+ void __iomem *cpu_base = gic_data_cpu_base(gic_data[gic_nr]);
u32 val = 0;
Well, this is rubbish. I need to check the gic_nr is valid. Will update ...
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: gic: Add a cpu map for each GIC instance
On 30/07/15 08:19, Marc Zyngier wrote: On Wed, 29 Jul 2015 17:10:45 +0100 Thomas Gleixner t...@linutronix.de wrote: On Wed, 29 Jul 2015, Jon Hunter wrote: Cc'ing Marc ... Thanks for looping me in. The gic_init_bases() function initialises an array that stores the mapping between the GIC and CPUs. This array is a global array that is unconditionally initialised on every call to gic_init_bases(). Although, it is not common for there to be more than one GIC instance, there are some devices that do support nested GIC controllers and gic_init_bases() can be called more than once. A 2nd call to gic_init_bases() will clear the previous CPU mapping and will only setup the mapping again for CPU0. This is because for child GIC controllers there is most likely only one recipient of the interrupt. Fix this by moving the CPU mapping array to the GIC chip data structure so that it is initialised for each GIC instance separately. It is assumed that the bL switcher code is only interested in the root or primary GIC instance. This feels very odd. If you have a secondary GIC, it is cascaded into the primary one, and I don't see why you would need to manage the affinity of the interrupt for the secondary GIC. The only thing that matters is the affinity of interrupts in the primary one, and this is what the bl switcher is dealing with. To me, it looks like the bug is to even try to compute an affinity for a GIC that is not the primary one, and keeping it around doesn't seem to make much sense. Or am I overlooking something? I mentioned to Russell (sorry you were not copied), that I am looking at a use-case where the secondary GIC is actually a router that can route interrupts to the main CPU cluster (via the primary GIC) or to another CPU. So it turns out that I do see a use for being able to configure the cpu map for the secondary as well. In this case the set_affinity would be use to direct the interrupt to either the main cluster or the other CPU. Please note that this patch does not address configuring the map for the secondary GIC. I am trying to think about the best way to handle this. I guess it could done via device-tree or we could have a API, gic_set_cpu_map(), that would allow you to set it. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] irqchip: gic: Add a cpu map for each GIC instance
On 30/07/15 09:20, Marc Zyngier wrote: On 29/07/15 20:27, Jon Hunter wrote: On 29/07/15 19:33, Russell King - ARM Linux wrote: On Wed, Jul 29, 2015 at 03:43:04PM +0100, Jon Hunter wrote: The gic_init_bases() function initialises an array that stores the mapping between the GIC and CPUs. This array is a global array that is unconditionally initialised on every call to gic_init_bases(). Although, it is not common for there to be more than one GIC instance, there are some devices that do support nested GIC controllers and gic_init_bases() can be called more than once. A 2nd call to gic_init_bases() will clear the previous CPU mapping and will only setup the mapping again for CPU0. This is because for child GIC controllers there is most likely only one recipient of the interrupt. Fix this by moving the CPU mapping array to the GIC chip data structure so that it is initialised for each GIC instance separately. It is assumed that the bL switcher code is only interested in the root or primary GIC instance. Does it make sense to expose the per-CPU-ness of the non-primary GIC? If they are chained off a primary GIC SPI interrupt, then all IRQs on the secondary GIC are routed to the same CPU that the SPI on the primary GIC is routed to. I am looking at a use-case where there is a secondary GIC and the secondary GIC is used as a interrupt router between the main CPU cluster and another CPU. So in this case the mapping of a secondary is still of interest. This patch does not address setting up the secondary mapping, but avoids a secondary GIC overwriting the primary map (which we don't want). Other features like the PPIs and SGIs in the secondary CPU should also be ignored - they probably aren't used anyway. Yes, agree. I have to say though... are the 1020 IRQs that the primary GIC provides really not enough? What insane hardware needs more than 1020 IRQs? Ha. I guess some realview boards for a start ... # grep -r gic_init(1 arch/arm/ arch/arm/mach-realview/realview_pb1176.c:gic_init(1, IRQ_PB1176_GIC_START, arch/arm/mach-realview/realview_eb.c:gic_init(1, 96, __io_address(REALVIEW_EB_GIC_DIST_BASE), arch/arm/mach-realview/realview_pb11mp.c:gic_init(1, IRQ_PB11MP_GIC_START, Different use case. These are development boards with a relatively modular design, where the SoC may or may not have a GIC by itself. When it has one, you end up with the on-board GIC being a secondary one. Yes, I understand that the use-case may be different, but I highlighted this as a use where the primary map would be overwritten as the driver is today. I never thought someone would quote these designs as an example to follow... ;-) I can't say if this example was followed, but nevertheless hardware designers certainly do get creative ;-) So we need to ensure the primary cpu map does not get overwritten by a secondary and I have a case where the secondary map is of interest. So are ok with this? Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 2/2] irqchip/gic: Ensure gic_cpu_if_up/down() programs correct GIC instance
Commit 3228950621d9 (irqchip: gic: Preserve gic V2 bypass bits in cpu
ctrl register) added a new function, gic_cpu_if_up(), to program the
GIC CPU_CTRL register. This function assumes that there is only one GIC
instance present and hence always uses the chip data for the primary GIC
controller. Although it is not common for there to be a secondary, some
devices do support a secondary. Therefore, fix this by passing
gic_cpu_if_up() a pointer to the appropriate chip data structure.
Similarly, the function gic_cpu_if_down() only assumes that there is a
single GIC instance present. Update this function so that an instance
number is passed for the appropriate GIC and return an error code on
failure. The vexpress TC2 (which has a single GIC) is currently the only
user of this function and so update it accordingly. Note that because the
TC2 only has a single GIC, the call to gic_cpu_if_down() should always
be successful.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
V2 changes:
- Rebased on v4.2-rc4
- Added test to ensure GIC instance is valid to gic_cpu_if_down() and
updated gic_cpu_if_down() to return an error code on failure.
arch/arm/mach-vexpress/tc2_pm.c | 2 +-
drivers/irqchip/irq-gic.c | 18 --
include/linux/irqchip/arm-gic.h | 2 +-
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index b3328cd46c33..1aa4ccece69f 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -80,7 +80,7 @@ static void tc2_pm_cpu_powerdown_prepare(unsigned int cpu,
unsigned int cluster)
* to the CPU by disabling the GIC CPU IF to prevent wfi
* from completing execution behind power controller back
*/
- gic_cpu_if_down();
+ gic_cpu_if_down(0);
}
static void tc2_pm_cluster_powerdown_prepare(unsigned int cluster)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index dc5090543eca..fdd1b0e6e5c3 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -355,9 +355,9 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
return mask;
}
-static void gic_cpu_if_up(void)
+static void gic_cpu_if_up(struct gic_chip_data *gic)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
+ void __iomem *cpu_base = gic_data_cpu_base(gic);
u32 bypass = 0;
/*
@@ -425,17 +425,23 @@ static void gic_cpu_init(struct gic_chip_data *gic)
gic_cpu_config(dist_base, NULL);
writel_relaxed(GICC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
- gic_cpu_if_up();
+ gic_cpu_if_up(gic);
}
-void gic_cpu_if_down(void)
+int gic_cpu_if_down(unsigned int gic_nr)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
+ void __iomem *cpu_base;
u32 val = 0;
+ if (gic_nr = MAX_GIC_NR)
+ return -EINVAL;
+
+ cpu_base = gic_data_cpu_base(gic_data[gic_nr]);
val = readl(cpu_base + GIC_CPU_CTRL);
val = ~GICC_ENABLE;
writel_relaxed(val, cpu_base + GIC_CPU_CTRL);
+
+ return 0;
}
#ifdef CONFIG_CPU_PM
@@ -571,7 +577,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
dist_base + GIC_DIST_PRI + i * 4);
writel_relaxed(GICC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
- gic_cpu_if_up();
+ gic_cpu_if_up(gic_data[gic_nr]);
}
static int gic_notifier(struct notifier_block *self, unsigned long cmd,
void *v)
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 9de976b4f9a7..da6aced1105e 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -99,7 +99,7 @@ void gic_set_irqchip_flags(unsigned long flags);
void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *,
u32 offset, struct device_node *);
void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
-void gic_cpu_if_down(void);
+int gic_cpu_if_down(unsigned int gic_nr);
static inline void gic_init(unsigned int nr, int start,
void __iomem *dist , void __iomem *cpu)
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH V2 1/2] irqchip/gic: Only allow the primary GIC to set the CPU map
The gic_init_bases() function initialises an array that stores the mapping
between the GIC and CPUs. This array is a global array that is
unconditionally initialised on every call to gic_init_bases(). Although,
it is not common for there to be more than one GIC instance, there are
some devices that do support nested GIC controllers and gic_init_bases()
can be called more than once.
A 2nd call to gic_init_bases() will clear the previous CPU mapping and
will only setup the mapping again for the CPU calling gic_init_bases().
Fix this by only allowing the CPU map to be configured for the primary GIC.
For secondary GICs the CPU map is not relevant because these GICs do not
directly route the interrupts to the main CPU(s) but to other GICs or
devices.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
V2 changes:
- Rebased on v4.2-rc4
drivers/irqchip/irq-gic.c | 43 +--
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4dd88264dff5..dc5090543eca 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -401,19 +401,26 @@ static void gic_cpu_init(struct gic_chip_data *gic)
int i;
/*
-* Get what the GIC says our CPU mask is.
+* Setting up the CPU map is only relevant for the primary GIC
+* because any nested/secondary GICs do not directly interface
+* with the CPU(s).
*/
- BUG_ON(cpu = NR_GIC_CPU_IF);
- cpu_mask = gic_get_cpumask(gic);
- gic_cpu_map[cpu] = cpu_mask;
+ if (gic == gic_data[0]) {
+ /*
+* Get what the GIC says our CPU mask is.
+*/
+ BUG_ON(cpu = NR_GIC_CPU_IF);
+ cpu_mask = gic_get_cpumask(gic);
+ gic_cpu_map[cpu] = cpu_mask;
- /*
-* Clear our mask from the other map entries in case they're
-* still undefined.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- if (i != cpu)
- gic_cpu_map[i] = ~cpu_mask;
+ /*
+* Clear our mask from the other map entries in case they're
+* still undefined.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ if (i != cpu)
+ gic_cpu_map[i] = ~cpu_mask;
+ }
gic_cpu_config(dist_base, NULL);
@@ -930,13 +937,6 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
}
/*
-* Initialize the CPU interface map to all CPUs.
-* It will be refined as each CPU probes its ID.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- gic_cpu_map[i] = 0xff;
-
- /*
* Find out how many interrupts are supported.
* The GIC only supports up to 1020 interrupt sources.
*/
@@ -981,6 +981,13 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
return;
if (gic_nr == 0) {
+ /*
+* Initialize the CPU interface map to all CPUs.
+* It will be refined as each CPU probes its ID.
+* This is only necessary for the primary GIC.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ gic_cpu_map[i] = 0xff;
#ifdef CONFIG_SMP
set_smp_cross_call(gic_raise_softirq);
register_cpu_notifier(gic_cpu_notifier);
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] irqchip/gic: Ensure gic_cpu_if_up/down() programs correct GIC instance
Commit 3228950621d9 (irqchip: gic: Preserve gic V2 bypass bits in cpu
ctrl register) added a new function, gic_cpu_if_up(), to program the
GIC CPU_CTRL register. This function assumes that there is only one GIC
instance present and hence always uses the chip data for the primary GIC
controller. Although it is not common for there to be a secondary, some
devices do support a secondary. Therefore, fix this by passing
gic_cpu_if_up() a pointer to the appropriate chip data structure.
Similarly, the function gic_cpu_if_down() only assumes that there is a
single GIC instance present. Update this function so that an instance
number is passed for the appropriate GIC. The vexpress TC2 (which has
a single GIC) is currently the only user of this function and so update
it accordingly.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
I was hoping to make the gic_cpu_if_up/down function more symmetric as we
discussed but it is not possible to pass the gic_nr to gic_cpu_if_up()
from all the places called without making more changes. However, given
that gic_cpu_if_up() is a local function and gic_cpu_if_down() is public,
may be it does not matter too much.
arch/arm/mach-vexpress/tc2_pm.c | 2 +-
drivers/irqchip/irq-gic.c | 14 +++---
include/linux/irqchip/arm-gic.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-vexpress/tc2_pm.c b/arch/arm/mach-vexpress/tc2_pm.c
index b3328cd46c33..1aa4ccece69f 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -80,7 +80,7 @@ static void tc2_pm_cpu_powerdown_prepare(unsigned int cpu,
unsigned int cluster)
* to the CPU by disabling the GIC CPU IF to prevent wfi
* from completing execution behind power controller back
*/
- gic_cpu_if_down();
+ gic_cpu_if_down(0);
}
static void tc2_pm_cluster_powerdown_prepare(unsigned int cluster)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7566fe259d27..cf9aca22120f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -356,10 +356,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
return mask;
}
-static void gic_cpu_if_up(void)
+static void gic_cpu_if_up(struct gic_chip_data *gic)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
- void __iomem *dist_base = gic_data_dist_base(gic_data[0]);
+ void __iomem *cpu_base = gic_data_cpu_base(gic);
+ void __iomem *dist_base = gic_data_dist_base(gic);
u32 bypass = 0;
/*
@@ -451,12 +451,12 @@ static void gic_cpu_init(struct gic_chip_data *gic)
}
writel_relaxed(GICC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
- gic_cpu_if_up();
+ gic_cpu_if_up(gic);
}
-void gic_cpu_if_down(void)
+void gic_cpu_if_down(unsigned int gic_nr)
{
- void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
+ void __iomem *cpu_base = gic_data_cpu_base(gic_data[gic_nr]);
u32 val = 0;
val = readl(cpu_base + GIC_CPU_CTRL);
@@ -608,7 +608,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
}
writel_relaxed(GICC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
- gic_cpu_if_up();
+ gic_cpu_if_up(gic_data[gic_nr]);
}
static int gic_notifier(struct notifier_block *self, unsigned long cmd,
void *v)
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index f52a9024be9a..9728bf4ca31b 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -100,7 +100,7 @@ struct device_node;
void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *,
u32 offset, struct device_node *);
void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
-void gic_cpu_if_down(void);
+void gic_cpu_if_down(unsigned int gic_nr);
static inline void gic_init(unsigned int nr, int start,
void __iomem *dist , void __iomem *cpu)
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/2] irqchip/gic: Only allow the primary GIC to set the CPU map
The gic_init_bases() function initialises an array that stores the mapping
between the GIC and CPUs. This array is a global array that is
unconditionally initialised on every call to gic_init_bases(). Although,
it is not common for there to be more than one GIC instance, there are
some devices that do support nested GIC controllers and gic_init_bases()
can be called more than once.
A 2nd call to gic_init_bases() will clear the previous CPU mapping and
will only setup the mapping again for the CPU calling gic_init_bases().
Fix this by only allowing the CPU map to be configured for the primary GIC.
For secondary GICs the CPU map is not relevant because these GICs do not
directly route the interrupts to the main CPU(s) but to other GICs or
devices.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
drivers/irqchip/irq-gic.c | 43 +--
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a530d9a9b810..7566fe259d27 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -416,19 +416,26 @@ static void gic_cpu_init(struct gic_chip_data *gic)
int i;
/*
-* Get what the GIC says our CPU mask is.
+* Setting up the CPU map is only relevant for the primary GIC
+* because any nested/secondary GICs do not directly interface
+* with the CPU(s).
*/
- BUG_ON(cpu = NR_GIC_CPU_IF);
- cpu_mask = gic_get_cpumask(gic);
- gic_cpu_map[cpu] = cpu_mask;
+ if (gic == gic_data[0]) {
+ /*
+* Get what the GIC says our CPU mask is.
+*/
+ BUG_ON(cpu = NR_GIC_CPU_IF);
+ cpu_mask = gic_get_cpumask(gic);
+ gic_cpu_map[cpu] = cpu_mask;
- /*
-* Clear our mask from the other map entries in case they're
-* still undefined.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- if (i != cpu)
- gic_cpu_map[i] = ~cpu_mask;
+ /*
+* Clear our mask from the other map entries in case they're
+* still undefined.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ if (i != cpu)
+ gic_cpu_map[i] = ~cpu_mask;
+ }
gic_cpu_config(dist_base, NULL);
@@ -977,13 +984,6 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
}
/*
-* Initialize the CPU interface map to all CPUs.
-* It will be refined as each CPU probes its ID.
-*/
- for (i = 0; i NR_GIC_CPU_IF; i++)
- gic_cpu_map[i] = 0xff;
-
- /*
* Find out how many interrupts are supported.
* The GIC only supports up to 1020 interrupt sources.
*/
@@ -1028,6 +1028,13 @@ void __init gic_init_bases(unsigned int gic_nr, int
irq_start,
return;
if (gic_nr == 0) {
+ /*
+* Initialize the CPU interface map to all CPUs.
+* It will be refined as each CPU probes its ID.
+* This is only necessary for the primary GIC.
+*/
+ for (i = 0; i NR_GIC_CPU_IF; i++)
+ gic_cpu_map[i] = 0xff;
#ifdef CONFIG_SMP
set_smp_cross_call(gic_raise_softirq);
register_cpu_notifier(gic_cpu_notifier);
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] irqchip/gic: Ensure gic_cpu_if_up/down() programs correct GIC instance
On 30/07/15 17:51, Marc Zyngier wrote:
On 30/07/15 17:26, Jon Hunter wrote:
Commit 3228950621d9 (irqchip: gic: Preserve gic V2 bypass bits in cpu
ctrl register) added a new function, gic_cpu_if_up(), to program the
GIC CPU_CTRL register. This function assumes that there is only one GIC
instance present and hence always uses the chip data for the primary GIC
controller. Although it is not common for there to be a secondary, some
devices do support a secondary. Therefore, fix this by passing
gic_cpu_if_up() a pointer to the appropriate chip data structure.
Similarly, the function gic_cpu_if_down() only assumes that there is a
single GIC instance present. Update this function so that an instance
number is passed for the appropriate GIC. The vexpress TC2 (which has
a single GIC) is currently the only user of this function and so update
it accordingly.
Signed-off-by: Jon Hunter jonath...@nvidia.com
---
I was hoping to make the gic_cpu_if_up/down function more symmetric as we
discussed but it is not possible to pass the gic_nr to gic_cpu_if_up()
from all the places called without making more changes. However, given
that gic_cpu_if_up() is a local function and gic_cpu_if_down() is public,
may be it does not matter too much.
arch/arm/mach-vexpress/tc2_pm.c | 2 +-
drivers/irqchip/irq-gic.c | 14 +++---
include/linux/irqchip/arm-gic.h | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/arm/mach-vexpress/tc2_pm.c
b/arch/arm/mach-vexpress/tc2_pm.c
index b3328cd46c33..1aa4ccece69f 100644
--- a/arch/arm/mach-vexpress/tc2_pm.c
+++ b/arch/arm/mach-vexpress/tc2_pm.c
@@ -80,7 +80,7 @@ static void tc2_pm_cpu_powerdown_prepare(unsigned int cpu,
unsigned int cluster)
* to the CPU by disabling the GIC CPU IF to prevent wfi
* from completing execution behind power controller back
*/
-gic_cpu_if_down();
+gic_cpu_if_down(0);
}
static void tc2_pm_cluster_powerdown_prepare(unsigned int cluster)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7566fe259d27..cf9aca22120f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -356,10 +356,10 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
return mask;
}
-static void gic_cpu_if_up(void)
+static void gic_cpu_if_up(struct gic_chip_data *gic)
{
-void __iomem *cpu_base = gic_data_cpu_base(gic_data[0]);
-void __iomem *dist_base = gic_data_dist_base(gic_data[0]);
+void __iomem *cpu_base = gic_data_cpu_base(gic);
+void __iomem *dist_base = gic_data_dist_base(gic);
Which tree is that against? I don't have a dist_base in mainline...
It is based upon linux-next. I can rebase on the current mainline if you
want them for v4.2.
Jon
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
