RE: [PATCH v7 2/5] OMAP: McBSP: Modify macros/functions API for easy cache access
-Original Message- From: Tony Lindgren [mailto:t...@atomide.com] Sent: Tuesday, December 15, 2009 1:06 AM To: Jarkko Nikula Cc: Janusz Krzysztofik; Varadarajan, Charu Latha; Peter Ujfalusi; linux-omap@vger.kernel.org Subject: Re: [PATCH v7 2/5] OMAP: McBSP: Modify macros/functions API for easy cache access * Jarkko Nikula jhnik...@gmail.com [091214 03:12]: On Mon, 14 Dec 2009 11:11:27 +0100 Janusz Krzysztofik jkrzy...@tis.icnet.pl wrote: If these functions are obsolete and going to be removed, I don't think it could be of any importance whether they are modified before removal or not. Otherwise, a solution seems simple to me: you submit a patch that addresses all concearns, yours, Tony's (BTW, have you already reviewed the drivers as Tony suggested?), maybe others. Then, after your patch is accepted for inclusion and it appears in conflict with this series still not applied for any reason (possibly waiting for your changes if that decided), Jarkko, or Tony, or anyone else pointed out by Tony, decides which one goes first and which is going to be refreshed on top of the other. Does it sound like a good plan for you? I would favor the Janusz's set going in first since it will solve and help the in-tree problems without making out-of-tree use any worse than currently. - Fixes register access in polled I/O functions (out-of-tree use) - Fixes McBSP register corruption noted on Amstrad Delta - McBSP register caching could help the PM development Fixing any other issues in polled I/O API belongs to another context than this patch set and IMO is easier to handle after this set since the patch 1/5 already fixes the register access width. Sounds good to me. Tony, As pointed by you, the McBSP driver is having broken APIs. Please share your plans wrt McBSP driver. 1) If there are plans to review and clean up the code, we will focus on the same. 2) Else if the above is planned sometime in the future, can you consider the patch for fixing McBSP poll mode (http://patchwork.kernel.org/patch/54896/) as it is important to have a working code in place? If okay, I shall post a new version of the mentioned patch. Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 2/5] OMAP: McBSP: Modify macros/functions API for easy cache access
snip
@@ -612,7 +597,6 @@ EXPORT_SYMBOL(omap_mcbsp_stop);
int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
{
struct omap_mcbsp *mcbsp;
- void __iomem *base;
if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR %s: Invalid id (%d)\n,
__func__, id + 1);
@@ -620,28 +604,25 @@ int omap_mcbsp_pollwrite(unsigned int id
}
mcbsp = id_to_mcbsp_ptr(id);
- base = mcbsp-io_base;
- OMAP_MCBSP_WRITE(base, DXR1, buf);
+ MCBSP_WRITE(mcbsp, DXR1, buf);
OMAP3/4 allows 32 bit data access also.
How is it taken care?
Not at all in case of those several particular changes above.
I'm pretty sure
that just replacing old OMAP_MCBSP_WRITE(base, ...) with new
MCBSP_WRITE(mcbsp, ...) changed nothing in terms of OMAP3/4
32-bit data
access. Again, beyond the scope.
Please refer to
http://patchwork.kernel.org/patch/54896/ for more details.
Quoting Tony's words:
To me it smells like the all drivers using the the
omap_mcbsp_pollread/write are broken legacy drivers.
So maybe we should just remove these two functions?
If we really want to keep these around, we should review
the drivers using these functions.
Please have a look at patch 1/5, there readw()/writew() has
been replaced with
OMAP_MCBSP_READ()/_WRITE() inside
omap_mcbsp_pollwrite()/_pollread(). If you
think there is something wrong with it, please comment that
patch, not this
one that changes nothing related here, I believe.
The point here is to review the driver before pushing any patch
touching these functions.
snip
RE: [PATCH v7 2/5] OMAP: McBSP: Modify macros/functions API for easy cache access
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of Janusz
Krzysztofik
Sent: Thursday, December 10, 2009 1:59 AM
To: Tony Lindgren
Cc: Jarkko Nikula; Peter Ujfalusi; linux-omap@vger.kernel.org
Subject: [PATCH v7 2/5] OMAP: McBSP: Modify macros/functions
API for easy cache access
OMAP_MCBSP_READ()/_WRITE() macros and
omap_mcbsp_read()/_write() functions
accept McBSP register base address as an argument. In order to support
caching, that must be replaced with an address of the
omap_mcbsp structure
that would provide addresses for both register AND cache access.
Since OMAP_ prefix seems obvious in macro names, drop it off
in order to
minimize line wrapping throughout the file.
Applies on top of patch 1 from this series:
[PATCH v7 1/5] OMAP: McBSP: Use macros for all register
read/write operations
Tested on OMAP1510 based Amstrad Delta using linux-omap for-next,
commit 82f1d8f22f2c65e70206e40a6f17688bf64a892c.
Compile-tested with omap_3430sdp_defconfig.
Signed-off-by: Janusz Krzysztofik jkrzy...@tis.icnet.pl
---
Changes since v3:
- modify API of omap_mcbsp_read()/_write() functions as well,
since those will
actually provide caching operations, not macros like before.
snip
@@ -313,19 +305,18 @@ static inline void omap34xx_mcbsp_reques
if (cpu_is_omap34xx()) {
u16 syscon;
- syscon = OMAP_MCBSP_READ(mcbsp-io_base, SYSCON);
+ syscon = MCBSP_READ(mcbsp, SYSCON);
syscon = ~(ENAWAKEUP | SIDLEMODE(0x03) |
CLOCKACTIVITY(0x03));
Would this driver get adpated to the hwmod framework? Then this
would be invalid.
if (mcbsp-dma_op_mode == MCBSP_DMA_MODE_THRESHOLD) {
syscon |= (ENAWAKEUP | SIDLEMODE(0x02) |
CLOCKACTIVITY(0x02));
- OMAP_MCBSP_WRITE(mcbsp-io_base, WAKEUPEN,
- XRDYEN | RRDYEN);
+ MCBSP_WRITE(mcbsp, WAKEUPEN, XRDYEN | RRDYEN);
} else {
syscon |= SIDLEMODE(0x01);
}
- OMAP_MCBSP_WRITE(mcbsp-io_base, SYSCON, syscon);
+ MCBSP_WRITE(mcbsp, SYSCON, syscon);
Ditto
}
}
@@ -337,7 +328,7 @@ static inline void omap34xx_mcbsp_free(s
if (cpu_is_omap34xx()) {
u16 syscon;
- syscon = OMAP_MCBSP_READ(mcbsp-io_base, SYSCON);
+ syscon = MCBSP_READ(mcbsp, SYSCON);
syscon = ~(ENAWAKEUP | SIDLEMODE(0x03) |
CLOCKACTIVITY(0x03));
/*
* HW bug workaround - If no_idle mode is
taken, we need to
@@ -345,12 +336,12 @@ static inline void omap34xx_mcbsp_free(s
* device will not hit retention anymore.
*/
syscon |= SIDLEMODE(0x02);
- OMAP_MCBSP_WRITE(mcbsp-io_base, SYSCON, syscon);
+ MCBSP_WRITE(mcbsp, SYSCON, syscon);
Ditto
syscon = ~(SIDLEMODE(0x03));
- OMAP_MCBSP_WRITE(mcbsp-io_base, SYSCON, syscon);
+ MCBSP_WRITE(mcbsp, SYSCON, syscon);
Ditto
- OMAP_MCBSP_WRITE(mcbsp-io_base, WAKEUPEN, 0);
+ MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
}
}
snip
/*
* Worst case: CLKSRG*2 = 8000khz: (1/8000) * 2 * 2 usec
@@ -543,18 +531,18 @@ void omap_mcbsp_start(unsigned int id, i
if (idle) {
/* Start frame sync */
- w = OMAP_MCBSP_READ(io_base, SPCR2);
- OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 7));
+ w = MCBSP_READ(mcbsp, SPCR2);
+ MCBSP_WRITE(mcbsp, SPCR2, w | (1 7));
}
if (cpu_is_omap2430() || cpu_is_omap34xx()) {
Why not 44xx?
/* Release the transmitter and receiver */
- w = OMAP_MCBSP_READ(io_base, XCCR);
+ w = MCBSP_READ(mcbsp, XCCR);
w = ~(tx ? XDISABLE : 0);
- OMAP_MCBSP_WRITE(io_base, XCCR, w);
- w = OMAP_MCBSP_READ(io_base, RCCR);
+ MCBSP_WRITE(mcbsp, XCCR, w);
+ w = MCBSP_READ(mcbsp, RCCR);
w = ~(rx ? RDISABLE : 0);
- OMAP_MCBSP_WRITE(io_base, RCCR, w);
+ MCBSP_WRITE(mcbsp, RCCR, w);
}
/* Dump McBSP Regs */
@@ -565,7 +553,6 @@ EXPORT_SYMBOL(omap_mcbsp_start);
void omap_mcbsp_stop(unsigned int id, int tx, int rx)
{
struct omap_mcbsp *mcbsp;
- void __iomem *io_base;
int idle;
u16 w;
@@ -575,35 +562,33 @@ void omap_mcbsp_stop(unsigned int id, in
}
mcbsp = id_to_mcbsp_ptr(id);
- io_base = mcbsp-io_base;
/* Reset transmitter */
tx = 1;
if (cpu_is_omap2430() || cpu_is_omap34xx()) {
Why not 44xx?
- w = OMAP_MCBSP_READ(io_base, XCCR);
+ w = MCBSP_READ(mcbsp, XCCR);
w |= (tx ? XDISABLE : 0);
-
RE: [PATCH v2] OMAP3 : Fix I2C lockup during timeout/error cases
-Original Message- From: G, Manjunath Kondaiah Sent: Thursday, December 10, 2009 6:49 PM To: linux-omap@vger.kernel.org Cc: G, Manjunath Kondaiah; Menon, Nishanth; Sonasath, Moiz; Krishnamoorthy, Balaji T; Varadarajan, Charu Latha; George, Harith Subject: [PATCH v2] OMAP3 : Fix I2C lockup during timeout/error cases Current OMAP3 I2C driver code does not follow the correct sequence for soft reset. Due to this, lock up issues are reported during timeout/error cases. This patch fixes above issue by disabling I2C controller as per OMAP3430 TRM for soft reset. As per TRM, I2C controller needs to be disabled as a first step during soft reset. Here is correct soft reset sequence: a. Ensure that the module is disabled (clear the I2Ci.I2C_CON[15] I2C_EN bit to 0). b. Set the I2Ci.I2C_SYSC[1] SRST bit to 1. c. Enable the module by setting I2Ci.I2C_CON[15] I2C_EN bit to 1. d. Check the I2Ci.I2C_SYSS[0] RDONE bit until it is set to 1 to indicate the software reset is complete. v1 : Initital version v2 : Removed delay since time taken for disabling I2C controller is zero jiffies Tested on Zoom2 and Zoom3 Signed-off-by: Manjunatha GK manj...@ti.com Signed-off-by: George, Harithhar...@ti.com Tested on 3430SDP too. Acked-by: Charulatha V ch...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3
-Original Message- From: Tony Lindgren [mailto:t...@atomide.com] Sent: Friday, November 13, 2009 2:41 AM To: Varadarajan, Charu Latha Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin Subject: Re: [PATCH V2] FIX OMAP3:McBSP poll read and write for OMAP3 * ch...@ti.com ch...@ti.com [091019 23:41]: omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access McBSP registers as 16-bit registers. In OMAP3, McBSP registers (DRR_REG and DXR_REG) are limited to 32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is not allowed and can corrupt register content. This patch adds omap3_mcbsp_pollwrite and omap3_mcbsp_pollread functions for OMAP3 cpu, inorder to perform 32 bit access of above mentioned McBSP registers. How about making the functions generic, I don't think we want to have separate omap1_mcbsp_pollread, omap2_mcbsp_pollread, omap3_mcbsp_pollread.. If it's not obvious how to implement it for some omap, just return an error based on the CPU type. My v1 patch handles this in a single generic API. [http://www.spinics.net/lists/linux-omap/msg19074.html and http://patchwork.kernel.org/patch/53631/] As per the review comments received for v1 patch, v2 patch is modified to have new API omap3_mcbsp_pollread/write in addition to omap_mcbsp_pollread/write APIs. Review comments for V1 mentioned that API signartures should not be changed from 16bit to 32 bit input parameters. If the API needs to be made generic, API signature has to be modified as given below (as in patch v1): -int omap_mcbsp_pollread(unsigned int id, u16 *buf) +int omap_mcbsp_pollread(unsigned int id, u32 *buf) If agreed for this generic API change, I shall post v3 patch which would handle cputypes dynamically inside the API Regards, Tony Signed-off-by: Charulatha V ch...@ti.com Signed-off-by: Syed Rafiuddin rafiuddin.s...@ti.com --- arch/arm/plat-omap/include/mach/mcbsp.h |4 + arch/arm/plat-omap/mcbsp.c | 92 ++- 2 files changed, 95 insertions(+), 1 deletions(-) diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h index 0c5f937..b0f9260 100644 --- a/arch/arm/plat-omap/include/mach/mcbsp.h +++ b/arch/arm/plat-omap/include/mach/mcbsp.h @@ -406,6 +406,10 @@ void omap_mcbsp_set_spi_mode(unsigned int id, const struct omap_mcbsp_spi_cfg * /* Polled read/write functions */ int omap_mcbsp_pollread(unsigned int id, u16 * buf); int omap_mcbsp_pollwrite(unsigned int id, u16 buf); + +int omap3_mcbsp_pollread(unsigned int id, u32 *buf); +int omap3_mcbsp_pollwrite(unsigned int id, u32 buf); + int omap_mcbsp_set_io_type(unsigned int id, omap_mcbsp_io_type_t io_type); #endif snip-- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V2] [OMAP] GPIO Module disable when all pins are inactive
Subject: [PATCH V2] [OMAP] GPIO Module disable when all pins
are inactive
From: Charulatha V ch...@ti.com
This patch disables a GPIO module when all pins of a GPIO
module are inactive (clock gating forced at module level) and
enables the module when any gpio in the module is requested.
The module is enabled only when mod_usage indicates that no GPIO
in that module is currently active and the GPIO being requested
is the 1st one to be active in that module.
[sp] This para reads quite confusing. The subject talks of disable
but this para indicates process for 'enable'.
Shall change the subject to GPIO module enable/disable.
Each module would be disabled in omap_gpio_free() API when all
GPIOs in a particular module becomes inactive. The module is
re-enabled in omap_gpio_request() API when a GPIO is requested
from the module that was previously disabled.
Since individual GPIO's bookkeeping is introduced automatically
in this patch(mod_usage), the same is used in
omap_set_gpio_debounce()
[sp] Is book-keeping 'automatically introduced' or added in this
patch?
The intention of the patch is not to introduce bookkeeping. But the
implementation has brought in bookkeeping concept modulewise.
I shall change it to added instead of automatically introduced.
omap_set_gpio_debounce_time() APIs to ensure that the gpio being
used is actually requested prior to being used (Nishant Menon's
n...@ti.com Suggestion)
Higher layer keeps track of GPIOs individually. This patch
introduces bookkeeping information, modulewise in lower layer
since disabling clock is done at module level. GPIO module level
details are specific to hardware and introducing APIs in higher
level layer to handle them might not be correct. Hence GPIO module
level information (mod_usage) has to be handled only in
low-level layer.
[sp] Again the description seems to be quite confusing between the
higher layer and lower layer contexts.
I shall remove the above para and keep it as:
GPIO module level details are specific to hardware and hence introducing
this patch in low level layer (plat-omap/gpio.c).
Signed-off-by: Charulatha V ch...@ti.com
Acked-by: Nishanth Menon n...@ti.com
---
arch/arm/plat-omap/gpio.c | 35 +++
1 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 4c35f9f..5ee6a60 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -199,6 +199,7 @@ struct gpio_bank {
struct gpio_chip chip;
struct clk *dbck;
u32 dbck_enable_mask;
+u32 mod_usage;
};
#define METHOD_MPUIO0
@@ -691,6 +692,12 @@ void omap_set_gpio_debounce(int gpio,
int enable)
reg += OMAP24XX_GPIO_DEBOUNCE_EN;
#endif
+if ((cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())
+ (!(bank-mod_usage l))) {
[sp] Is the AND operation really needed?
Yes, inorder to check if this GPIO was requested before calling
omap_set_gpio_debounce
+printk(KERN_ERR GPIO not requested\n);
+return;
+}
+
Snip
reg += OMAP4_GPIO_DEBOUNCINGTIME;
@@ -1219,6 +1232,16 @@ static int omap_gpio_request(struct
gpio_chip *chip, unsigned offset)
__raw_writel(__raw_readl(reg) | (1 offset), reg);
}
#endif
+if (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx()) {
+u32 ctrl;
[sp] This should move into next if where it is used.
Ok.
+if (!bank-mod_usage) {
+ctrl = __raw_readl(bank-base +
Snip--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive
#endif
+ if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+ if (!bank-gpio_status) {
+ ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+ /* Module is enabled, clocks are not gated */
+ ctrl = 0xFFFE;
+ __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+ }
+ bank-gpio_status |= 1 offset;
+ }
why do this every time a gpio is enabled? why not do this iff gpio was
never used before.. how about the following:
The module is enabled only when gpio_status indicates that no GPIO
in that module is currently active and the GPIO being requested is the 1st
one
to be active in that module.
Each module would be disabled in free() API when all GPIOs in a particular
module
becomes inactive. The module is re-enabled in request() API when a GPIO is
requested from the module that was previously disabled.
Thanks.
Welcome
if (!bank-gpio_status (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is enabled, clocks are not gated */
ctrl = 0xFFFE;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
bank-gpio_status |= 1 offset;
Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)?
either the gpio_status should be under a #ifdef for 34xx when defining
or it should be usable by all. what it does now is do both.
gpio_status is not used under #ifdef for 34xx. It is used only with
cpu_is_omap
(34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why?
But I don't see that approach in LO. For eg., usage of dbck_enable_mask is
used only with cpu_is_omap and is not declared under #ifdef.
my proposal is to allow gpio_status to be usable by ALL OMAPs - maybe
OMAP1 also could also use it.. I cannot comment - but it does look to
have scope of usage beyond omap2/3/4 series?
Even though OMAP1 supports the same feature, I am not including it as I
cannot test it and I am not sure about it in OMAP1. For 24xx, 34xx 44xx,
the registers used and offsets are all the same. So the same code is reused. --
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [OMAP] GPIO Module is reset during initialization
Felipe Balbi had written, on 10/23/2009 05:56 PM, the following:
On Fri, Oct 23, 2009 at 09:25:29PM +0530, ch...@ti.com wrote:
From: Charulatha V ch...@ti.com
During initialization, GPIO module is reset using soft reset bit
of SYSCONFIG register
Signed-off-by: Charulatha V ch...@ti.com
---
arch/arm/plat-omap/gpio.c | 12 +++-
1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 2304a5d..4579650 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -21,6 +21,7 @@
#include linux/err.h
#include linux/clk.h
#include linux/io.h
+#include linux/delay.h
#include mach/hardware.h
#include asm/irq.h
@@ -1670,7 +1671,7 @@ static int __init _omap_gpio_init(void)
}
#endif
for (i = 0; i gpio_bank_count; i++) {
-int j, gpio_count = 16;
+int j, gpio_count = 16, attempt = 0;
decrementing is better, so:
Okay
attempt = 1000
please move attempt out of here to the {} for 3430.. Warning for OMAP1.
Okay
bank = gpio_bank[i];
spin_lock_init(bank-lock);
@@ -1698,6 +1699,15 @@ static int __init _omap_gpio_init(void)
static const u32 non_wakeup_gpios[] = {
0xe203ffc0, 0x08700040
};
+
+/* Software Reset of GPIO module */
+__raw_writel(0x0002, bank-base + OMAP24XX_GPIO_SYSCONFIG);
+while (((__raw_readl(bank-base + OMAP24XX_GPIO_SYSSTATUS)
+ 0x1) == 0) attempt 5) {
attemp)
+udelay(1);
i guess cpu_relax() is better here.
I guess cpu_relax is not required because this part of code is called only from
board file during boot-up. Hence this would be the only code to be executed.
+attempt++;
attempt--;
cant we improve this code as following:
{
u8 attempts = 25;
/* Software Reset of GPIO module */
__raw_writel(0x0002, bank-base
+ OMAP24XX_GPIO_SYSCONFIG);
/* wait for reset to be done */
while (((__raw_readl(bank-base +
OMAP24XX_GPIO_SYSSTATUS) 0x1) == 0)
attempts) {
cpu_relax();
if (attempts % 5)
udelay(1);
attempts--;
}
allows the kernel to do somethin else while we also ensure we have a 5
usec guarenteed delay before giving up..
Doesn't modulo operation cost more in terms of performance? Any specific
reason for specific 5 microseconds? --
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive
sinipped bank-gpio_status |= 1 offset; Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)? either the gpio_status should be under a #ifdef for 34xx when defining or it should be usable by all. what it does now is do both. gpio_status is not used under #ifdef for 34xx. It is used only with cpu_is_omap (34xx/24xx/44xx). Should we use both #ifdef and cpu_is_omap together? Why? But I don't see that approach in LO. For eg., usage of dbck_enable_mask is used only with cpu_is_omap and is not declared under #ifdef. Please see [1] saved_datain is an example of why I think gpio.c could go thru a cleanup ;) already in #ifdef in line 1908, in line 1925, we add a new #ifdef under a #ifdef :D.. err... Ok this piece of code is not perfect.. Looking onto lines 1908 1925, I accept that the gpio.c code is not perfect. But they are nothing to do with my patch. I guess this is the same at many places in most of the drivers and someone has to take up the job of cleaning up. Regards, Nishanth Menon .[1] http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=arch/arm/plat-omap/gpio.c;h=487bea7b5605fe366064d792d0c9cc8aed1eac63;hb=0bbf5337f2f2957775051a3caf60b66d3306c815#l174 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [OMAP] GPIO Module is reset during initialization
-snippet
attemp)
+udelay(1);
i guess cpu_relax() is better here.
I guess cpu_relax is not required because this part of code is called only
from
board file during boot-up. Hence this would be the only code to be
executed.
What is your opinion on this?
+attempt++;
attempt--;
cant we improve this code as following:
{
u8 attempts = 25;
/* Software Reset of GPIO module */
__raw_writel(0x0002, bank-base
+ OMAP24XX_GPIO_SYSCONFIG);
/* wait for reset to be done */
while (((__raw_readl(bank-base +
OMAP24XX_GPIO_SYSSTATUS) 0x1) == 0)
attempts) {
cpu_relax();
if (attempts % 5)
udelay(1);
attempts--;
}
allows the kernel to do somethin else while we also ensure we have a 5
usec guarenteed delay before giving up..
Doesn't modulo operation cost more in terms of performance? Any specific
reason for specific 5 microseconds?
You could replace it with operator if you like and use 2^x multiples..
I am just sticking 5 us there based on your original code..
so the same logic over here I suppose.. unless I missed something?
I was using attempts as 5, as my intension was to attempt only 5 times and not
in terms of usec as I could not find any details in any document for maximum
time for the bit to get set/reset. According to your code, it would attempt to
read
the register 25 times with a delay of 5 microseconds min during worst case
scenario.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [OMAP] GPIO Module is reset during initialization
snippet- i guess cpu_relax() is better here. I guess cpu_relax is not required because this part of code is called only from board file during boot-up. Hence this would be the only code to be executed. What is your opinion on this? Cpu_relax is still the better way of doing it. Please clarify why. allows the kernel to do somethin else while we also ensure we have a 5 usec guarenteed delay before giving up.. Doesn't modulo operation cost more in terms of performance? Any specific reason for specific 5 microseconds? You could replace it with operator if you like and use 2^x multiples.. I am just sticking 5 us there based on your original code.. so the same logic over here I suppose.. unless I missed something? I was using attempts as 5, as my intension was to attempt only 5 times and not in terms of usec as I could not find any details in any document for maximum time for the bit to get set/reset. According to your code, it would attempt to read the register 25 times with a delay of 5 microseconds min during worst case scenario. Please find the h/w timeout value and resubmit the patch.. As of now, I do not have any information on these details. Please share any details if you can. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] [OMAP] GPIO Module disable if all pins inactive
Varadarajan, Charu Latha had written, on 10/23/2009 10:55 AM, the following:
From: Charulatha V ch...@ti.com
This patch disables a GPIO module when all the pins of GPIO
module are inactive (clock gating forced at module level) and
enables the module when any gpio in the module is requested.
Signed-off-by: Charulatha V ch...@ti.com
---
arch/arm/plat-omap/gpio.c | 22 ++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index cdc2a58..2304a5d 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -194,6 +194,7 @@ struct gpio_bank {
spinlock_t lock;
struct gpio_chip chip;
struct clk *dbck;
+ u32 gpio_status;
please rename this as gpio_usage?
okay
maybe OMAP1 could also benefit out of this..
};
#define METHOD_MPUIO 0
@@ -1080,6 +1081,7 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
{
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+ u32 ctrl = 0;
Remove this to the {} no point in wasting stack space when you dont need
to + you will generate warning for OMAP1 platforms.
spin_lock_irqsave(bank-lock, flags);
@@ -1097,6 +1099,15 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
__raw_writel(__raw_readl(reg) | (1 offset), reg);
}
#endif
+ if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+ if (!bank-gpio_status) {
+ ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+ /* Module is enabled, clocks are not gated */
+ ctrl = 0xFFFE;
+ __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+ }
+ bank-gpio_status |= 1 offset;
+ }
why do this every time a gpio is enabled? why not do this iff gpio was
never used before.. how about the following:
The module is enabled only when gpio_status indicates that no GPIO
in that module is currently active and the GPIO being requested is the 1st one
to be active in that module.
Each module would be disabled in free() API when all GPIOs in a particular
module
becomes inactive. The module is re-enabled in request() API when a GPIO is
requested from the module that was previously disabled.
if (!bank-gpio_status (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is enabled, clocks are not gated */
ctrl = 0xFFFE;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
bank-gpio_status |= 1 offset;
Why to touch gpio_status if not used (for other than 34xx/24xx/44xx cases)?
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1106,6 +1117,7 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
{
struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
unsigned long flags;
+ u32 ctrl = 0;
used just once - move it to the {} + warning to OMAP1
spin_lock_irqsave(bank-lock, flags);
#ifdef CONFIG_ARCH_OMAP16XX
@@ -1123,6 +1135,15 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
__raw_writel(1 offset, reg);
}
#endif
+ if (cpu_is_omap24xx() || cpu_is_omap34xx() || cpu_is_omap44xx()) {
+ bank-gpio_status = ~(1 offset);
+ if (!bank-gpio_status) {
+ ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
+ /* Module is disabled, clocks are gated */
+ ctrl |= 1;
+ __raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
+ }
+ }
how about this:
bank-gpio_status = ~(1 offset);
if (!bank-gpio_status (cpu_is_omap24xx() || cpu_is_omap34xx() ||
cpu_is_omap44xx())) {
u32 ctrl = __raw_readl(bank-base + OMAP24XX_GPIO_CTRL);
/* Module is disabled, clocks are gated */
ctrl |= 1;
__raw_writel(ctrl, bank-base + OMAP24XX_GPIO_CTRL);
}
Why to touch gpio_status if not used (for other than 24xx/34xx/44xx cases)?
_reset_gpio(bank, bank-chip.base + offset);
spin_unlock_irqrestore(bank-lock, flags);
}
@@ -1700,6 +1721,7 @@ static int __init _omap_gpio_init(void)
gpio_count = 32;
}
#endif
+ bank-gpio_status = 0;
/* REVISIT eventually switch from OMAP-specific gpio structs
* over to the generic ones
*/
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] OMAP3: Fix McBSP poll read and write for 32bit reg access
-Original Message-
From: G, Manjunath Kondaiah
Sent: Wednesday, October 14, 2009 4:01 PM
To: 'vimal singh'; Varadarajan, Charu Latha
Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin
Subject: RE: [PATCH] OMAP3: Fix McBSP poll read and write for
32bit reg access
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of vimal singh
Sent: Wednesday, October 14, 2009 3:23 PM
To: Varadarajan, Charu Latha
Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin
Subject: Re: [PATCH] OMAP3: Fix McBSP poll read and write for
32bit reg access
base = mcbsp-io_base;
/* if frame sync error - clear the error */
- if (readw(base + OMAP_MCBSP_REG_SPCR1) RSYNC_ERR) {
+ if (OMAP_MCBSP_READ(base, SPCR1) RSYNC_ERR) {
/* clear error */
- writew(readw(base + OMAP_MCBSP_REG_SPCR1)
(~RSYNC_ERR),
- base + OMAP_MCBSP_REG_SPCR1);
+ OMAP_MCBSP_WRITE(base, SPCR1,
OMAP_MCBSP_READ(base, SPCR1)
+(~RSYNC_ERR));
/* resend */
return -1;
Return value can be a meaningful macro with proper value?
} else {
/* wait for recieve confirmation */
int attemps = 0;
- while (!(readw(base + OMAP_MCBSP_REG_SPCR1)
RRDY)) {
- if (attemps++ 1000) {
- writew(readw(base +
OMAP_MCBSP_REG_SPCR1)
- (~RRST),
- base +
OMAP_MCBSP_REG_SPCR1);
+ while (!(OMAP_MCBSP_READ(base, SPCR1) RRDY)) {
+ if (attemps++ 1) {
+ OMAP_MCBSP_WRITE(base, SPCR1,
+
Why attemps changed from 1000 to 1?
Ooops.. That's a typo, but critical error. Will do the needful.
-Manjunath
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] OMAP3: Fix McBSP poll read and write for 32bit reg access
__
Subject: RE: [PATCH] OMAP3: Fix McBSP poll read and write for
32bit reg access
Charu,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Varadarajan, Charu Latha
Sent: Wednesday, October 14, 2009 3:00 PM
To: linux-omap@vger.kernel.org
Cc: Varadarajan, Charu Latha; Syed, Rafiuddin
Subject: [PATCH] OMAP3: Fix McBSP poll read and write for
32bit reg access
omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
McBSP registers as 16-bit registers.
The McBSP registers (DRR_REG and DXR_REG) are limited to
32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
not allowed and can corrupt register content.
This patch modifies omap_mcbsp_pollwrite and
omap_mcbsp_pollread functions to do 32 bit access for above
mentioned McBSP registers. Data accepted by these
functions is also modified to 32-bit.
Signed-off-by: Charulatha V ch...@ti.com
Signed-off-by: Syed Rafiuddin rafiuddin.s...@ti.com
---
arch/arm/plat-omap/include/mach/mcbsp.h |4 +-
arch/arm/plat-omap/mcbsp.c | 46
++
-
2 files changed, 23 insertions(+), 27 deletions(-)
diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h
b/arch/arm/plat-
omap/include/mach/mcbsp.h
index 7e9cae3..05b0d8d 100644
--- a/arch/arm/plat-omap/include/mach/mcbsp.h
+++ b/arch/arm/plat-omap/include/mach/mcbsp.h
@@ -455,8 +455,8 @@ int
omap_mcbsp_spi_master_recv_word_poll(unsigned int
id, u32 * word);
void omap_mcbsp_set_spi_mode(unsigned int id, const struct
omap_mcbsp_spi_cfg * spi_cfg);
/* Polled read/write functions */
-int omap_mcbsp_pollread(unsigned int id, u16 * buf);
-int omap_mcbsp_pollwrite(unsigned int id, u16 buf);
+int omap_mcbsp_pollread(unsigned int id, u32 *buf);
+int omap_mcbsp_pollwrite(unsigned int id, u32 buf);
If the issue is with DRR_REG and DXR_REG, why are you
changing the signature of the generic API. You can handle
these registers as a exceptions in the code.
If 32 bit data could be read and written on McBSP, why should we restrict
poll read and
poll write functions only for 16 bit data read and write?
If these API's are only accessing the DXR and DRR regs then this is OK.
int omap_mcbsp_set_io_type(unsigned int id, omap_mcbsp_io_type_t
io_type);
#endif
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 88ac976..1f278a2 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -613,7 +613,7 @@ void omap_mcbsp_stop(unsigned int id,
int tx, int rx)
EXPORT_SYMBOL(omap_mcbsp_stop);
/* polled mcbsp i/o operations */
-int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
+int omap_mcbsp_pollwrite(unsigned int id, u32 buf)
{
struct omap_mcbsp *mcbsp;
void __iomem *base;
@@ -626,26 +626,24 @@ int omap_mcbsp_pollwrite(unsigned int
id, u16 buf)
mcbsp = id_to_mcbsp_ptr(id);
base = mcbsp-io_base;
- writew(buf, base + OMAP_MCBSP_REG_DXR1);
+ OMAP_MCBSP_WRITE(base, DXR, buf);
Why do you need this? Is writew()/readw() not doing 16 bit
operations ?
The intension is to use 32 bit access and not 16 bit access
Use readl() / writel() then.
Yes, OMAP_MCBSP_WRITE macro makes use of readl() / writel() for OMAP3
/* if frame sync error - clear the error */
- if (readw(base + OMAP_MCBSP_REG_SPCR2) XSYNC_ERR) {
+ if (OMAP_MCBSP_READ(base, SPCR2) XSYNC_ERR) {
/* clear error */
- writew(readw(base + OMAP_MCBSP_REG_SPCR2)
(~XSYNC_ERR),
- base + OMAP_MCBSP_REG_SPCR2);
+ OMAP_MCBSP_WRITE(base, SPCR2,
OMAP_MCBSP_READ(base , SPCR2)
+(~XSYNC_ERR));
/* resend */
return -1;
} else {
/* wait for transmit confirmation */
int attemps = 0;
- while (!(readw(base + OMAP_MCBSP_REG_SPCR2) XRDY)) {
+ while (!(OMAP_MCBSP_READ(base, SPCR2) XRDY)) {
if (attemps++ 1000) {
- writew(readw(base +
OMAP_MCBSP_REG_SPCR2)
- (~XRST),
- base + OMAP_MCBSP_REG_SPCR2);
+ OMAP_MCBSP_WRITE(base, SPCR2,
+ OMAP_MCBSP_READ(base,
SPCR2) (~XRST));
udelay(10);
- writew(readw(base +
OMAP_MCBSP_REG_SPCR2) |
- (XRST),
- base + OMAP_MCBSP_REG_SPCR2);
+ OMAP_MCBSP_WRITE(base, SPCR2
RE: [PATCH] OMAP3: Fix McBSP poll read and write for 32bit reg access
-Original Message-
From: linux-omap-ow...@vger.kernel.org
[mailto:linux-omap-ow...@vger.kernel.org] On Behalf Of vimal singh
Sent: Wednesday, October 14, 2009 3:23 PM
To: Varadarajan, Charu Latha
Cc: linux-omap@vger.kernel.org; Syed, Rafiuddin
Subject: Re: [PATCH] OMAP3: Fix McBSP poll read and write for
32bit reg access
On Wed, Oct 14, 2009 at 3:00 PM, ch...@ti.com wrote:
omap_mcbsp_pollwrite and omap_mcbsp_pollread functions access
McBSP registers as 16-bit registers.
The McBSP registers (DRR_REG and DXR_REG) are limited to
32-bit data accesses (L4 Interconnect). 16-bit and 8-bit is
not allowed and can corrupt register content.
This patch modifies omap_mcbsp_pollwrite and
omap_mcbsp_pollread functions to do 32 bit access for above
mentioned McBSP registers. Data accepted by these
functions is also modified to 32-bit.
Signed-off-by: Charulatha V ch...@ti.com
Signed-off-by: Syed Rafiuddin rafiuddin.s...@ti.com
---
arch/arm/plat-omap/include/mach/mcbsp.h |4 +-
arch/arm/plat-omap/mcbsp.c | 46
++-
2 files changed, 23 insertions(+), 27 deletions(-)
diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h
b/arch/arm/plat-omap/include/mach/mcbsp.h
index 7e9cae3..05b0d8d 100644
--- a/arch/arm/plat-omap/include/mach/mcbsp.h
+++ b/arch/arm/plat-omap/include/mach/mcbsp.h
@@ -455,8 +455,8 @@ int
omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 * word);
void omap_mcbsp_set_spi_mode(unsigned int id, const struct
omap_mcbsp_spi_cfg * spi_cfg);
/* Polled read/write functions */
-int omap_mcbsp_pollread(unsigned int id, u16 * buf);
-int omap_mcbsp_pollwrite(unsigned int id, u16 buf);
+int omap_mcbsp_pollread(unsigned int id, u32 *buf);
+int omap_mcbsp_pollwrite(unsigned int id, u32 buf);
int omap_mcbsp_set_io_type(unsigned int id,
omap_mcbsp_io_type_t io_type);
#endif
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 88ac976..1f278a2 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -613,7 +613,7 @@ void omap_mcbsp_stop(unsigned int id,
int tx, int rx)
EXPORT_SYMBOL(omap_mcbsp_stop);
/* polled mcbsp i/o operations */
-int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
+int omap_mcbsp_pollwrite(unsigned int id, u32 buf)
{
struct omap_mcbsp *mcbsp;
void __iomem *base;
@@ -626,26 +626,24 @@ int omap_mcbsp_pollwrite(unsigned int
id, u16 buf)
mcbsp = id_to_mcbsp_ptr(id);
base = mcbsp-io_base;
- writew(buf, base + OMAP_MCBSP_REG_DXR1);
+ OMAP_MCBSP_WRITE(base, DXR, buf);
/* if frame sync error - clear the error */
- if (readw(base + OMAP_MCBSP_REG_SPCR2) XSYNC_ERR) {
+ if (OMAP_MCBSP_READ(base, SPCR2) XSYNC_ERR) {
/* clear error */
- writew(readw(base + OMAP_MCBSP_REG_SPCR2)
(~XSYNC_ERR),
- base + OMAP_MCBSP_REG_SPCR2);
+ OMAP_MCBSP_WRITE(base, SPCR2,
OMAP_MCBSP_READ(base , SPCR2)
remove extra space: 'base ,'
checkpatch.pl should catch these kind of issues.
checkpatch.pl has not captured this error
Anyways, I will modify the same in my next version of patch
-vimal
+(~XSYNC_ERR));
/* resend */
return -1;
} else {
/* wait for transmit confirmation */
int attemps = 0;
- while (!(readw(base + OMAP_MCBSP_REG_SPCR2)
XRDY)) {
+ while (!(OMAP_MCBSP_READ(base, SPCR2) XRDY)) {
if (attemps++ 1000) {
- writew(readw(base +
OMAP_MCBSP_REG_SPCR2)
- (~XRST),
- base + OMAP_MCBSP_REG_SPCR2);
+ OMAP_MCBSP_WRITE(base, SPCR2,
+
OMAP_MCBSP_READ(base, SPCR2) (~XRST));
udelay(10);
- writew(readw(base +
OMAP_MCBSP_REG_SPCR2) |
- (XRST),
- base + OMAP_MCBSP_REG_SPCR2);
+ OMAP_MCBSP_WRITE(base, SPCR2,
+
OMAP_MCBSP_READ(base, SPCR2) | (XRST));
udelay(10);
dev_err(mcbsp-dev, Could
not write to
McBSP%d
Register\n, mcbsp-id);
@@ -658,7 +656,7 @@ int omap_mcbsp_pollwrite(unsigned int
id, u16 buf)
}
EXPORT_SYMBOL(omap_mcbsp_pollwrite);
-int omap_mcbsp_pollread(unsigned int id, u16 *buf)
+int omap_mcbsp_pollread(unsigned int id, u32 *buf)
{
struct omap_mcbsp *mcbsp;
void __iomem *base;
@@ -671,24 +669,22
RE: [PATCH v2 0/4] RTC-MSECURE: platform data structure defined
Tony, Can you upstream these series of patches if there are no comments? From: Varadarajan, Charu Latha Sent: Monday, August 24, 2009 8:36 PM To: linux-omap@vger.kernel.org Cc: t...@atomide.com; davi...@pacbell.net; sa...@linux.intel.com; p_gortma...@yahoo.com; Varadarajan, Charu Latha Subject: [PATCH v2 0/4] RTC-MSECURE: platform data structure defined Review comments are incorporated. This series contains patches which does the following. 1. define TWL4030 rtc platform data which has msecure gpio line info 2. initialise msecure gpio line from rtc driver 3. twl core file passes rtc platform data from board file to rtc driver 4. Board files handle mux configuration of msecure gpio line This series applies on top of linux-2.6.31-rc5. CharuLatha (4): [PATCH 1/4] RTC platform data structure defined [PATCH 2/4] RTC msecure initialization [PATCH 3/4] handling rtc platform data [PATCH 4/4] board files do msecure line mux configuration Signed-off-by: Charulatha V ch...@ti.com --- include/linux/i2c/twl4030.h |5 + drivers/rtc/rtc-twl4030.c | 38 +- drivers/mfd/twl4030-core.c|2 +- arch/arm/mach-omap2/board-2430sdp.c |7 +++ arch/arm/mach-omap2/board-3430sdp.c | 18 +- arch/arm/mach-omap2/board-ldp.c | 19 +++ arch/arm/mach-omap2/mux.c |5 + arch/arm/plat-omap/include/mach/mux.h |2 ++ 8 files changed, 93 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Issue with setting vdd2_opp level to 3 through OMAP3 SRF API's
Hello,
When trying to make vdd2_opp resource to VDD2_OPP3 level using
resource_request(arch\arm\plat-omap\resource.c), it is observed that vdd2_opp
is always put to VDD2_OPP2 level.
Code Snippet:
For the above, set_opp() in arch\arm\mach-omap2\resource34xx.c is called with a
target level of 3. The code flow is as given below:
/*** Code Snippet Start/
else if (resp == vdd2_resp) {
tput = target_level;
/* Convert the tput in KiB/s to Bus frequency in MHz */
req_l3_freq = (tput * 1000)/4;
for (ind = 2; ind = MAX_VDD2_OPP; ind++){
if ((l3_opps + ind)-rate = req_l3_freq) {
target_level = ind;
break;
}
}
/* Set the highest OPP possible */
if (ind MAX_VDD2_OPP)
target_level = ind-1;
resource_set_opp_level(VDD2_OPP, target_level, 0);
}
/*** Code Snippet End/
Analysis:
During execution,
tout = 3;
req_l3_freq = 750;
when ind = 2, if ((l3_opps + ind)-rate = req_l3_freq) == (83M 750) which
in turn makes target_level = 2.
Hence while requesting for target_level of 3, vdd2_opp is made to target_level
= 2, which is incorrect.
Issues observed:
The above code should have been made for mapping L3 freq 83MHz to VDD2_OPP2 and
L3 freq 166MHz to VDD2_OPP3. But the above code would always make vdd2_opp to
VDD2_OPP2 level. However, it is observed that, if we change the vdd2_opp to 3
using sysfs, it seems to be setting to level 3. I am wondering if there are any
issues with sysfs entries since same is working with resource frame work API's.
Please let me know if my understanding is wrong.
Thanks Regards,
V Charu Latha--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Issue with setting vdd2_opp level to 3 through OMAP3 SRF API's
Hello,
The format of calling resource_rerquest for vdd2 opp setting is
resource_request(vdd2_opp, dev, r);
where r is the throughput.
Example, in set_opp, if vdd1 opp is = 3, vdd2 opp is being set to 3.
The above means resource_request expects targeted level
for all types of OPP/Freq resources except for vdd2_opp.
Only for vdd2_opp we need to give the throughput required
and not the targeted level. Is that correct?
API needs to be generic. If my above understanding is correct,
why resource_request is expecting an unique kind of
input parameter, only for vdd2_opp resource?
Thanks,
V Charu Latha
___
From: linux-omap-ow...@vger.kernel.org [linux-omap-ow...@vger.kernel.org] On
Behalf Of Varadarajan, Charu Latha
Sent: Thursday, September 17, 2009 2:55 PM
To: linux-omap@vger.kernel.org
Subject: Issue with setting vdd2_opp level to 3 through OMAP3 SRF API's
Hello,
When trying to make vdd2_opp resource to VDD2_OPP3 level using
resource_request(arch\arm\plat-omap\resource.c), it is observed that vdd2_opp
is always put to VDD2_OPP2 level.
Code Snippet:
For the above, set_opp() in arch\arm\mach-omap2\resource34xx.c is called with a
target level of 3. The code flow is as given below:
/*** Code Snippet Start/
else if (resp == vdd2_resp) {
tput = target_level;
/* Convert the tput in KiB/s to Bus frequency in MHz */
req_l3_freq = (tput * 1000)/4;
for (ind = 2; ind = MAX_VDD2_OPP; ind++){
if ((l3_opps + ind)-rate = req_l3_freq) {
target_level = ind;
break;
}
}
/* Set the highest OPP possible */
if (ind MAX_VDD2_OPP)
target_level = ind-1;
resource_set_opp_level(VDD2_OPP, target_level, 0);
}
/*** Code Snippet End/
Analysis:
During execution,
tout = 3;
req_l3_freq = 750;
when ind = 2, if ((l3_opps + ind)-rate = req_l3_freq) == (83M 750) which
in turn makes target_level = 2.
Hence while requesting for target_level of 3, vdd2_opp is made to target_level
= 2, which is incorrect.
Issues observed:
The above code should have been made for mapping L3 freq 83MHz to VDD2_OPP2 and
L3 freq 166MHz to VDD2_OPP3. But the above code would always make vdd2_opp to
VDD2_OPP2 level. However, it is observed that, if we change the vdd2_opp to 3
using sysfs, it seems to be setting to level 3. I am wondering if there are any
issues with sysfs entries since same is working with resource frame work API's.
Please let me know if my understanding is wrong.
Thanks Regards,
V Charu Latha
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] RTC MSECURE: msecure initialization
Vikram,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
Sent: Wednesday, August 26, 2009 12:01 AM
To: Varadarajan, Charu Latha; linux-omap@vger.kernel.org
Cc: t...@atomide.com; davi...@pacbell.net; sa...@linux.intel.com;
p_gortma...@yahoo.com
Subject: RE: [PATCH v2 2/4] RTC MSECURE: msecure initialization
Charu -- possible bug
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of
Varadarajan, Charu Latha
/*
+ * msecure line initialisation for TWL4030 RTC registers write access
+ */
+static int msecure_init(struct twl4030_rtc_platform_data *pdata)
+{
+int ret = 0;
+if (pdata == NULL)
+goto out;
+
+ret = gpio_request(pdata-msecure_gpio, msecure);
What about other boards like: Beagle, EVM, Pandora, Zoom2, RX51
What about SDP/LDP with OMAP ES2.1 and above
For these boards, there is no msecure_gpio defined.
What would be the value for msecure_gpio for these boards?
For the twl4030 based boards which don't have msecure gpio lines, the rtc
platform data
need to be null. The same was addressed in version 1 patch and it has been
agreed that
the default value is NULL and hence need not be initialized in board files.
This patch is tested on SDP boards with silicon version 3.1.
+if (ret) {
+pr_err(twl4030_rtc: can't reserve msecure GPIO:%d !\n
+RTC functionality will not be available\n,
+pdata-msecure_gpio);
+goto out;
+}
+/*
+ * TWL4030 will be in secure mode if msecure line from OMAP is low.
+ * Make msecure line high in order to change the TWL4030 RTC time
+ * and calender registers.
+ */
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 4/4] RTC MSECURE: mux configuration in board files
Vikram,
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
Sent: Tuesday, August 25, 2009 11:54 PM
To: Varadarajan, Charu Latha; linux-omap@vger.kernel.org
Cc: t...@atomide.com; davi...@pacbell.net; sa...@linux.intel.com;
p_gortma...@yahoo.com; Varadarajan, Charu Latha
Subject: RE: [PATCH v2 4/4] RTC MSECURE: mux configuration in board files
Charu
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of
ch...@ti.com
Sent: Monday, August 24, 2009 10:08 AM
To: linux-omap@vger.kernel.org
Cc: t...@atomide.com; davi...@pacbell.net; sa...@linux.intel.com;
p_gortma...@yahoo.com; Varadarajan,
Charu Latha
Subject: [PATCH v2 4/4] RTC MSECURE: mux configuration in board files
static struct i2c_board_info __initdata sdp2430_i2c_boardinfo[] = {
diff --git a/arch/arm/mach-omap2/board-3430sdp.c b/arch/arm/mach-omap2/board-
3430sdp.c
index 351e8c5..1bb79f5 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -54,7 +54,7 @@
#define ENABLE_VAUX3_DEDICATED 0x03
#define ENABLE_VAUX3_DEV_GRP0x20
-#define TWL4030_MSECURE_GPIO 22
+#define TWL4030_MSECURE_GPIO22
static int sdp3430_keymap[] = {
KEY(0, 0, KEY_LEFT),
@@ -574,8 +574,24 @@ static inline void board_smc91x_init(void)
#endif
+int sdp3430_rtc_init(void)
+{
+struct twl4030_rtc_platform_data sdp3430_rtc_data;
+/* msecure mux cfg */
+if (omap_type() == OMAP2_DEVICE_TYPE_GP
+omap_rev() OMAP3430_REV_ES2_0) {
What happens to msecure line on SDP/LDP with ES2.0 and above?
The default value is NULL, hence no need to reinitialize. FYI, the patch was
tested on
SDP boards with greater than ES2.0 version silicon.
Is it that Triton MSECURE line gets pulled up by default, and hence no need
for the patch?
Msecure initialization is not required for such boards.
+sdp3430_rtc_data.msecure_gpio = TWL4030_MSECURE_GPIO;
+sdp3430_twldata.rtc = sdp3430_rtc_data;
+return omap_cfg_reg(AF9_34XX_V1_X_GPIO22_OUT);
+} else
+return 0;
+};
+
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
p--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH 0/3] RTC platform data structure defined
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Kevin Hilman
Sent: Saturday, August 22, 2009 9:58 PM
To: Varadarajan, Charu Latha
Cc: linux-omap@vger.kernel.org; t...@atomide.com; davi...@pacbell.net;
sa...@linux.intel.com; p_gortma...@yahoo.com
Subject: Re: [RFC][PATCH 0/3] RTC platform data structure defined
ch...@ti.com writes:
This series contains patches which
1. define TWL4030 rtc platform data which has msecure gpio line
info
2. initialise msecure gpio line from rtc driver
3. twl core file passes rtc platform data from board file to rtc
driver
4. Board files handle mux configuration of msecure gpio line
This series applies on top of linux-2.6.31-rc5.
CharuLatha (4):
[PATCH 0/3] RTC platform data structure defined
Normally, PATCH 0/n is a summary of the other patches, and does not
include a patch itself.
I recommend using 'git format-patch' which will take care of the
formatting and the numbering for you.
Okay. I will take care hereafter.
[PATCH 1/3] RTC periodic interrupts enabling and msecure init
[PATCH 2/3] handling rtc platform data
[PATCH 3/3] board files do msecure line mux configuration
Structure twl4030_rtc_platform_data is defined with msecure_gpio in
it.
This structure would be initialised in board file.
RTC msecure line initialization happens in RTC driver.
The same structure can be used for any future enhancements in RTC.
Signed-off-by: Charulatha V ch...@ti.com
---
include/linux/i2c/twl4030.h |8
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/include/linux/i2c/twl4030.h b/include/linux/i2c/twl4030.h
index 15edfaa..6ae582b 100644
--- a/include/linux/i2c/twl4030.h
+++ b/include/linux/i2c/twl4030.h
@@ -341,6 +341,13 @@ struct twl4030_gpio_platform_data {
unsigned gpio, unsigned ngpio);
};
+struct twl4030_rtc_platform_data {
+ int msecure_gpio;
+ /* More structures/elements can be added
+* for future Triton RTC driver enhancements
+*/
very true, but does not need to be documented.
Agreed.
+};
+
struct twl4030_madc_platform_data {
int irq_line;
};
@@ -395,6 +402,7 @@ struct twl4030_platform_data {
unsignedirq_base, irq_end;
struct twl4030_bci_platform_data*bci;
struct twl4030_gpio_platform_data *gpio;
+ struct twl4030_rtc_platform_data*rtc;
struct twl4030_madc_platform_data *madc;
struct twl4030_keypad_data *keypad;
struct twl4030_usb_data *usb;
--
1.6.0.4
--
Kevin
--
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure init
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Kevin Hilman
Sent: Saturday, August 22, 2009 9:59 PM
To: Roger Quadros
Cc: Varadarajan, Charu Latha; linux-omap@vger.kernel.org;
t...@atomide.com; davi...@pacbell.net; sa...@linux.intel.com;
p_gortma...@yahoo.com
Subject: Re: [RFC][PATCH 1/3] RTC periodic interrupts enabling and msecure
init
Roger Quadros ext-roger.quad...@nokia.com writes:
ext ch...@ti.com wrote:
Triton2 RTC code changes for fixing periodic interrupt feature in RTC.
rtc-twlcore.c does initialisation of the msecure gpio pin. Board
files indicate msecure gpio line through twl4030 platform
data. twl4030-core.c passes this information to RTC driver.
Board files does msecure gpio mux configuration.
Signed-off-by: Charulatha V ch...@ti.com
Periodic interrupts and msecure are 2 different entities. I think they
should be implemented in different patches.
Agreed. The fix part of this should be a separated out with a
detailed description of both the problem and the fix.
Kevin
Okay.
---
drivers/rtc/rtc-twl4030.c | 63
-
1 files changed, 62 insertions(+), 1 deletions(-)
diff --git a/drivers/rtc/rtc-twl4030.c b/drivers/rtc/rtc-twl4030.c
index 9c8c70c..614adf0 100644
--- a/drivers/rtc/rtc-twl4030.c
+++ b/drivers/rtc/rtc-twl4030.c
@@ -29,7 +29,12 @@
#include linux/interrupt.h
#include linux/i2c/twl4030.h
+#include linux/gpio.h
+/*
+ * To find if the value is a power of two
+ */
+#define is_power_of_two(x)(!((x) ((x)-1)))
/*
* RTC block register offsets (use TWL_MODULE_RTC)
@@ -86,6 +91,37 @@
/*
--*/
/*
+ * msecure line initialisation for TWL4030 RTC registers write access
+ */
+static int msecure_init(struct twl4030_rtc_platform_data *pdata)
+{
+ int ret = 0;
+ if (pdata == NULL)
+ goto out;
+
+ ret = gpio_request(pdata-msecure_gpio, msecure);
+ if (ret 0) {
if (ret) should suffice
+ pr_err(twl4030_rtc: can't reserve msecure GPIO:%d !\n
+ RTC functionality will not be available\n,
+ pdata-msecure_gpio);
+ goto out;
+ }
+ /*
+ * TWL4030 will be in secure mode if msecure line from OMAP is low.
+ * Make msecure line high in order to change the TWL4030 RTC time
+ * and calender registers.
+ */
+ ret = gpio_direction_output(pdata-msecure_gpio, 1);
+ if (ret 0)
ditto
+ pr_err(twl4030_rtc: can't set msecure GPIO direction:%d !\n
+ RTC functionality will not be available\n,
+ pdata-msecure_gpio);
+
+out:
+ return ret;
+}
Agreed.
+
+/*
* Supports 1 byte read from TWL4030 RTC register.
*/
static int twl4030_rtc_read_u8(u8 *data, u8 reg)
@@ -128,7 +164,6 @@ static int set_rtc_irq_bit(unsigned char bit)
int ret;
val = rtc_irq_bits | bit;
- val = ~BIT_RTC_INTERRUPTS_REG_EVERY_M;
ret = twl4030_rtc_write_u8(val, REG_RTC_INTERRUPTS_REG);
if (ret == 0)
rtc_irq_bits = val;
@@ -318,6 +353,25 @@ out:
return ret;
}
+static int twl4030_rtc_irq_set_freq(struct device *dev, int freq)
+{
+ int ret, val = 1;
+ int regbit = 0;
+
+ if ((!is_power_of_2(freq)) || (freq 8) || (freq = 0))
+ return -EINVAL;
0 is valid freq. it means disable periodic interrupts.
+
+ while ((freq val) == 0) {
+ val = val 1;
+ regbit++;
+ }
as per your implementation, if user sets interrupt rate of 4 Hz then
you will set regbit to 2 which means interrupt every hour?
i.e. 0.00027 Hz. no?
+ ret = set_rtc_irq_bit(regbit);
+ if (ret)
+ dev_err(dev, rtc_irq_set_freq error %d\n, ret);
+
+ return ret;
+}
+
static irqreturn_t twl4030_rtc_interrupt(int irq, void *rtc)
{
unsigned long events = 0;
@@ -383,6 +437,7 @@ static struct rtc_class_ops twl4030_rtc_ops = {
.set_alarm = twl4030_rtc_set_alarm,
.alarm_irq_enable = twl4030_rtc_alarm_irq_enable,
.update_irq_enable = twl4030_rtc_update_irq_enable,
+ .irq_set_freq = twl4030_rtc_irq_set_freq,
IMHO this does not make sense.
twl4030 supports a max interrupt rate of 1 Hz (i.e. 1 sec). So you can
only support freq values of 0 and 1 i.e. 0 for disabled and 1 for 1
sec interrupt.
This functionality is already achieved by update_irq_enable.
-roger
--
Agreed that twl4030_rtc_update_irq_enable is enough if the periodic interrupt
is required only for 1 Hz.
If this is enough, then any ioctl call with RTC_IRQP_SET to twl4030 chip's RTC
can do only
1 Hz freq setting. This means that RTC_IRQP_SET is not required at all for
twl4030 chip's RTC
and only RTC_UIE_ON is enough. Please clarify.
The twl4030 chip also supports interrupt
