* [RFC][PATCH] ARM: OMAP: McBSP: Use register cache
@ 2009-08-12 10:39 Janusz Krzysztofik
2009-08-28 11:26 ` Janusz Krzysztofik
2009-11-14 2:06 ` Janusz Krzysztofik
0 siblings, 2 replies; 15+ messages in thread
From: Janusz Krzysztofik @ 2009-08-12 10:39 UTC (permalink / raw)
To: Tony Lindgren, Jarkko Nikula, Peter Ujfalusi
Cc: alsa-devel@alsa-project.org, linux-omap@vger.kernel.org,
linux-arm-kernel@lists.arm.linux.org.uk,
linux-kernel@vger.kernel.org
Change the way McBSP registers are updated: use cached values instead of
relying upon those read back from the device.
With this patch, I have finally managed to get rid of all random
playback/recording hangups on my OMAP1510 based Amstrad Delta (buggy?)
hardware. Before that, I could suspect that values read back from McBSP
registers before updating them happened to be errornous.
I think there is one important point that makes this patch worth of applying,
apart from my hardware quality. With the current code, if it ever happens to
any machine, no matter if OMAP1510 or newer, to read incorrect value from a
McBSP register, this wrong value will get written back without any checking.
That can lead to hardware damage if, for example, an input pin is turned into
output as a result.
Created against linux-2.6.31-rc5
Tested on Amstrad Delta
Signed-off-by: Janusz Krzysztofik
---
--- linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h.orig 2009-08-11 23:43:25.000000000 +0200
+++ linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h 2009-08-11 23:45:46.000000000 +0200
@@ -377,6 +377,8 @@ struct omap_mcbsp {
struct omap_mcbsp_platform_data *pdata;
struct clk *iclk;
struct clk *fclk;
+
+ struct omap_mcbsp_reg_cfg reg_cache;
};
extern struct omap_mcbsp **mcbsp_ptr;
extern int omap_mcbsp_count;
--- linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c.orig 2009-08-11 23:43:25.000000000 +0200
+++ linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c 2009-08-11 23:45:35.000000000 +0200
@@ -91,6 +91,7 @@ static void omap_mcbsp_dump_reg(u8 id)
static irqreturn_t omap_mcbsp_tx_irq_handler(int irq, void *dev_id)
{
struct omap_mcbsp *mcbsp_tx = dev_id;
+ struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_tx->reg_cache;
u16 irqst_spcr2;
irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2);
@@ -101,7 +102,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han
irqst_spcr2);
/* Writing zero to XSYNC_ERR clears the IRQ */
OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2,
- irqst_spcr2 & ~(XSYNC_ERR));
+ reg_cache->spcr2 &= ~(XSYNC_ERR));
} else {
complete(&mcbsp_tx->tx_irq_completion);
}
@@ -112,6 +113,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han
static irqreturn_t omap_mcbsp_rx_irq_handler(int irq, void *dev_id)
{
struct omap_mcbsp *mcbsp_rx = dev_id;
+ struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_rx->reg_cache;
u16 irqst_spcr1;
irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx->io_base, SPCR1);
@@ -122,7 +124,7 @@ static irqreturn_t omap_mcbsp_rx_irq_han
irqst_spcr1);
/* Writing zero to RSYNC_ERR clears the IRQ */
OMAP_MCBSP_WRITE(mcbsp_rx->io_base, SPCR1,
- irqst_spcr1 & ~(RSYNC_ERR));
+ reg_cache->spcr1 &= ~(RSYNC_ERR));
} else {
complete(&mcbsp_rx->tx_irq_completion);
}
@@ -167,6 +169,7 @@ static void omap_mcbsp_rx_dma_callback(i
void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
{
struct omap_mcbsp *mcbsp;
+ struct omap_mcbsp_reg_cfg *reg_cache;
void __iomem *io_base;
if (!omap_mcbsp_check_valid_id(id)) {
@@ -174,26 +177,27 @@ void omap_mcbsp_config(unsigned int id,
return;
}
mcbsp = id_to_mcbsp_ptr(id);
+ reg_cache = &mcbsp->reg_cache;
io_base = mcbsp->io_base;
dev_dbg(mcbsp->dev, "Configuring McBSP%d phys_base: 0x%08lx\n",
mcbsp->id, mcbsp->phys_base);
/* We write the given config */
- OMAP_MCBSP_WRITE(io_base, SPCR2, config->spcr2);
- OMAP_MCBSP_WRITE(io_base, SPCR1, config->spcr1);
- OMAP_MCBSP_WRITE(io_base, RCR2, config->rcr2);
- OMAP_MCBSP_WRITE(io_base, RCR1, config->rcr1);
- OMAP_MCBSP_WRITE(io_base, XCR2, config->xcr2);
- OMAP_MCBSP_WRITE(io_base, XCR1, config->xcr1);
- OMAP_MCBSP_WRITE(io_base, SRGR2, config->srgr2);
- OMAP_MCBSP_WRITE(io_base, SRGR1, config->srgr1);
- OMAP_MCBSP_WRITE(io_base, MCR2, config->mcr2);
- OMAP_MCBSP_WRITE(io_base, MCR1, config->mcr1);
- OMAP_MCBSP_WRITE(io_base, PCR0, config->pcr0);
+ OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = config->spcr2);
+ OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 = config->spcr1);
+ OMAP_MCBSP_WRITE(io_base, RCR2, reg_cache->rcr2 = config->rcr2);
+ OMAP_MCBSP_WRITE(io_base, RCR1, reg_cache->rcr1 = config->rcr1);
+ OMAP_MCBSP_WRITE(io_base, XCR2, reg_cache->xcr2 = config->xcr2);
+ OMAP_MCBSP_WRITE(io_base, XCR1, reg_cache->xcr1 = config->xcr1);
+ OMAP_MCBSP_WRITE(io_base, SRGR2, reg_cache->srgr2 = config->srgr2);
+ OMAP_MCBSP_WRITE(io_base, SRGR1, reg_cache->srgr1 = config->srgr1);
+ OMAP_MCBSP_WRITE(io_base, MCR2, reg_cache->mcr2 = config->mcr2);
+ OMAP_MCBSP_WRITE(io_base, MCR1, reg_cache->mcr1 = config->mcr1);
+ OMAP_MCBSP_WRITE(io_base, PCR0, reg_cache->pcr0 = config->pcr0);
if (cpu_is_omap2430() || cpu_is_omap34xx()) {
- OMAP_MCBSP_WRITE(io_base, XCCR, config->xccr);
- OMAP_MCBSP_WRITE(io_base, RCCR, config->rccr);
+ OMAP_MCBSP_WRITE(io_base, XCCR, reg_cache->xccr = config->xccr);
+ OMAP_MCBSP_WRITE(io_base, RCCR, reg_cache->rccr = config->rccr);
}
}
EXPORT_SYMBOL(omap_mcbsp_config);
@@ -232,6 +236,7 @@ EXPORT_SYMBOL(omap_mcbsp_set_io_type);
int omap_mcbsp_request(unsigned int id)
{
struct omap_mcbsp *mcbsp;
+ struct omap_mcbsp_reg_cfg *reg_cache;
int err;
if (!omap_mcbsp_check_valid_id(id)) {
@@ -239,6 +244,7 @@ int omap_mcbsp_request(unsigned int id)
return -ENODEV;
}
mcbsp = id_to_mcbsp_ptr(id);
+ reg_cache = &mcbsp->reg_cache;
spin_lock(&mcbsp->lock);
if (!mcbsp->free) {
@@ -261,8 +267,8 @@ int omap_mcbsp_request(unsigned int id)
* Make sure that transmitter, receiver and sample-rate generator are
* not running before activating IRQs.
*/
- OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, 0);
- OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, 0);
+ OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, reg_cache->spcr1 = 0);
+ OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, reg_cache->spcr2 = 0);
if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) {
/* We need to get IRQs here */
@@ -335,42 +341,38 @@ EXPORT_SYMBOL(omap_mcbsp_free);
void omap_mcbsp_start(unsigned int id, int tx, int rx)
{
struct omap_mcbsp *mcbsp;
+ struct omap_mcbsp_reg_cfg *reg_cache;
void __iomem *io_base;
int idle;
- u16 w;
if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
return;
}
mcbsp = id_to_mcbsp_ptr(id);
+ reg_cache = &mcbsp->reg_cache;
io_base = mcbsp->io_base;
- mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7;
- mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7;
+ mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7;
+ mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7;
- idle = !((OMAP_MCBSP_READ(io_base, SPCR2) |
- OMAP_MCBSP_READ(io_base, SPCR1)) & 1);
+ idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1);
if (idle) {
/* Start the sample generator */
- w = OMAP_MCBSP_READ(io_base, SPCR2);
- OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 6));
+ OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 6));
}
/* Enable transmitter and receiver */
- w = OMAP_MCBSP_READ(io_base, SPCR2);
- OMAP_MCBSP_WRITE(io_base, SPCR2, w | (tx & 1));
+ OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (tx & 1));
- w = OMAP_MCBSP_READ(io_base, SPCR1);
- OMAP_MCBSP_WRITE(io_base, SPCR1, w | (rx & 1));
+ OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 |= (rx & 1));
udelay(100);
if (idle) {
/* Start frame sync */
- w = OMAP_MCBSP_READ(io_base, SPCR2);
- OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 7));
+ OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 7));
}
/* Dump McBSP Regs */
@@ -381,9 +383,9 @@ EXPORT_SYMBOL(omap_mcbsp_start);
void omap_mcbsp_stop(unsigned int id, int tx, int rx)
{
struct omap_mcbsp *mcbsp;
+ struct omap_mcbsp_reg_cfg *reg_cache;
void __iomem *io_base;
int idle;
- u16 w;
if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
@@ -391,23 +393,20 @@ void omap_mcbsp_stop(unsigned int id, in
}
mcbsp = id_to_mcbsp_ptr(id);
+ reg_cache = &mcbsp->reg_cache;
io_base = mcbsp->io_base;
/* Reset transmitter */
- w = OMAP_MCBSP_READ(io_base, SPCR2);
- OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(tx & 1));
+ OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(tx & 1));
/* Reset receiver */
- w = OMAP_MCBSP_READ(io_base, SPCR1);
- OMAP_MCBSP_WRITE(io_base, SPCR1, w & ~(rx & 1));
+ OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 &= ~(rx & 1));
- idle = !((OMAP_MCBSP_READ(io_base, SPCR2) |
- OMAP_MCBSP_READ(io_base, SPCR1)) & 1);
+ idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1);
if (idle) {
/* Reset the sample rate generator */
- w = OMAP_MCBSP_READ(io_base, SPCR2);
- OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(1 << 6));
+ OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(1 << 6));
}
}
EXPORT_SYMBOL(omap_mcbsp_stop);
@@ -557,6 +556,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word);
int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word)
{
struct omap_mcbsp *mcbsp;
+ struct omap_mcbsp_reg_cfg *reg_cache;
void __iomem *io_base;
omap_mcbsp_word_length tx_word_length;
omap_mcbsp_word_length rx_word_length;
@@ -567,6 +567,7 @@ int omap_mcbsp_spi_master_xmit_word_poll
return -ENODEV;
}
mcbsp = id_to_mcbsp_ptr(id);
+ reg_cache = &mcbsp->reg_cache;
io_base = mcbsp->io_base;
tx_word_length = mcbsp->tx_word_length;
rx_word_length = mcbsp->rx_word_length;
@@ -580,9 +581,11 @@ int omap_mcbsp_spi_master_xmit_word_poll
spcr2 = OMAP_MCBSP_READ(io_base, SPCR2);
if (attempts++ > 1000) {
/* We must reset the transmitter */
- OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST));
+ OMAP_MCBSP_WRITE(io_base, SPCR2,
+ reg_cache->spcr2 &= (~XRST));
udelay(10);
- OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST);
+ OMAP_MCBSP_WRITE(io_base, SPCR2,
+ reg_cache->spcr2 |= XRST);
udelay(10);
dev_err(mcbsp->dev, "McBSP%d transmitter not "
"ready\n", mcbsp->id);
@@ -601,9 +604,11 @@ int omap_mcbsp_spi_master_xmit_word_poll
spcr1 = OMAP_MCBSP_READ(io_base, SPCR1);
if (attempts++ > 1000) {
/* We must reset the receiver */
- OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST));
+ OMAP_MCBSP_WRITE(io_base, SPCR1,
+ reg_cache->spcr1 &= (~RRST));
udelay(10);
- OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST);
+ OMAP_MCBSP_WRITE(io_base, SPCR1,
+ reg_cache->spcr1 |= RRST);
udelay(10);
dev_err(mcbsp->dev, "McBSP%d receiver not "
"ready\n", mcbsp->id);
@@ -623,6 +628,7 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit
int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word)
{
struct omap_mcbsp *mcbsp;
+ struct omap_mcbsp_reg_cfg *reg_cache;
u32 clock_word = 0;
void __iomem *io_base;
omap_mcbsp_word_length tx_word_length;
@@ -635,6 +641,7 @@ int omap_mcbsp_spi_master_recv_word_poll
}
mcbsp = id_to_mcbsp_ptr(id);
+ reg_cache = &mcbsp->reg_cache;
io_base = mcbsp->io_base;
tx_word_length = mcbsp->tx_word_length;
@@ -649,9 +656,11 @@ int omap_mcbsp_spi_master_recv_word_poll
spcr2 = OMAP_MCBSP_READ(io_base, SPCR2);
if (attempts++ > 1000) {
/* We must reset the transmitter */
- OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST));
+ OMAP_MCBSP_WRITE(io_base, SPCR2,
+ reg_cache->spcr2 &= (~XRST));
udelay(10);
- OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST);
+ OMAP_MCBSP_WRITE(io_base, SPCR2,
+ reg_cache->spcr2 |= XRST);
udelay(10);
dev_err(mcbsp->dev, "McBSP%d transmitter not "
"ready\n", mcbsp->id);
@@ -670,9 +679,11 @@ int omap_mcbsp_spi_master_recv_word_poll
spcr1 = OMAP_MCBSP_READ(io_base, SPCR1);
if (attempts++ > 1000) {
/* We must reset the receiver */
- OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST));
+ OMAP_MCBSP_WRITE(io_base, SPCR1,
+ reg_cache->spcr1 &= (~RRST));
udelay(10);
- OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST);
+ OMAP_MCBSP_WRITE(io_base, SPCR1,
+ reg_cache->spcr1 |= RRST);
udelay(10);
dev_err(mcbsp->dev, "McBSP%d receiver not "
"ready\n", mcbsp->id);
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-08-12 10:39 [RFC][PATCH] ARM: OMAP: McBSP: Use register cache Janusz Krzysztofik @ 2009-08-28 11:26 ` Janusz Krzysztofik 2009-11-14 2:06 ` Janusz Krzysztofik 1 sibling, 0 replies; 15+ messages in thread From: Janusz Krzysztofik @ 2009-08-28 11:26 UTC (permalink / raw) To: Tony Lindgren, Jarkko Nikula, Peter Ujfalusi Cc: alsa-devel@alsa-project.org, linux-omap@vger.kernel.org, Mark Brown, linux-arm-kernel@lists.arm.linux.org.uk, linux-kernel@vger.kernel.org Hi, No single comment on this idea? Thanks, Janusz -------------------------- On 2009-08-12 12:39, Janusz Krzysztofik wrote: > Change the way McBSP registers are updated: use cached values instead of > relying upon those read back from the device. > > With this patch, I have finally managed to get rid of all random > playback/recording hangups on my OMAP1510 based Amstrad Delta (buggy?) > hardware. Before that, I could suspect that values read back from McBSP > registers before updating them happened to be errornous. > > I think there is one important point that makes this patch worth of applying, > apart from my hardware quality. With the current code, if it ever happens to > any machine, no matter if OMAP1510 or newer, to read incorrect value from a > McBSP register, this wrong value will get written back without any checking. > That can lead to hardware damage if, for example, an input pin is turned into > output as a result. > > Created against linux-2.6.31-rc5 > > Tested on Amstrad Delta > > Signed-off-by: Janusz Krzysztofik > > --- > --- linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h.orig 2009-08-11 23:43:25.000000000 +0200 > +++ linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h 2009-08-11 23:45:46.000000000 +0200 > @@ -377,6 +377,8 @@ struct omap_mcbsp { > struct omap_mcbsp_platform_data *pdata; > struct clk *iclk; > struct clk *fclk; > + > + struct omap_mcbsp_reg_cfg reg_cache; > }; > extern struct omap_mcbsp **mcbsp_ptr; > extern int omap_mcbsp_count; > --- linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c.orig 2009-08-11 23:43:25.000000000 +0200 > +++ linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c 2009-08-11 23:45:35.000000000 +0200 > @@ -91,6 +91,7 @@ static void omap_mcbsp_dump_reg(u8 id) > static irqreturn_t omap_mcbsp_tx_irq_handler(int irq, void *dev_id) > { > struct omap_mcbsp *mcbsp_tx = dev_id; > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_tx->reg_cache; > u16 irqst_spcr2; > > irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); > @@ -101,7 +102,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > irqst_spcr2); > /* Writing zero to XSYNC_ERR clears the IRQ */ > OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, > - irqst_spcr2 & ~(XSYNC_ERR)); > + reg_cache->spcr2 &= ~(XSYNC_ERR)); > } else { > complete(&mcbsp_tx->tx_irq_completion); > } > @@ -112,6 +113,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > static irqreturn_t omap_mcbsp_rx_irq_handler(int irq, void *dev_id) > { > struct omap_mcbsp *mcbsp_rx = dev_id; > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_rx->reg_cache; > u16 irqst_spcr1; > > irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx->io_base, SPCR1); > @@ -122,7 +124,7 @@ static irqreturn_t omap_mcbsp_rx_irq_han > irqst_spcr1); > /* Writing zero to RSYNC_ERR clears the IRQ */ > OMAP_MCBSP_WRITE(mcbsp_rx->io_base, SPCR1, > - irqst_spcr1 & ~(RSYNC_ERR)); > + reg_cache->spcr1 &= ~(RSYNC_ERR)); > } else { > complete(&mcbsp_rx->tx_irq_completion); > } > @@ -167,6 +169,7 @@ static void omap_mcbsp_rx_dma_callback(i > void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > > if (!omap_mcbsp_check_valid_id(id)) { > @@ -174,26 +177,27 @@ void omap_mcbsp_config(unsigned int id, > return; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > > io_base = mcbsp->io_base; > dev_dbg(mcbsp->dev, "Configuring McBSP%d phys_base: 0x%08lx\n", > mcbsp->id, mcbsp->phys_base); > > /* We write the given config */ > - OMAP_MCBSP_WRITE(io_base, SPCR2, config->spcr2); > - OMAP_MCBSP_WRITE(io_base, SPCR1, config->spcr1); > - OMAP_MCBSP_WRITE(io_base, RCR2, config->rcr2); > - OMAP_MCBSP_WRITE(io_base, RCR1, config->rcr1); > - OMAP_MCBSP_WRITE(io_base, XCR2, config->xcr2); > - OMAP_MCBSP_WRITE(io_base, XCR1, config->xcr1); > - OMAP_MCBSP_WRITE(io_base, SRGR2, config->srgr2); > - OMAP_MCBSP_WRITE(io_base, SRGR1, config->srgr1); > - OMAP_MCBSP_WRITE(io_base, MCR2, config->mcr2); > - OMAP_MCBSP_WRITE(io_base, MCR1, config->mcr1); > - OMAP_MCBSP_WRITE(io_base, PCR0, config->pcr0); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = config->spcr2); > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 = config->spcr1); > + OMAP_MCBSP_WRITE(io_base, RCR2, reg_cache->rcr2 = config->rcr2); > + OMAP_MCBSP_WRITE(io_base, RCR1, reg_cache->rcr1 = config->rcr1); > + OMAP_MCBSP_WRITE(io_base, XCR2, reg_cache->xcr2 = config->xcr2); > + OMAP_MCBSP_WRITE(io_base, XCR1, reg_cache->xcr1 = config->xcr1); > + OMAP_MCBSP_WRITE(io_base, SRGR2, reg_cache->srgr2 = config->srgr2); > + OMAP_MCBSP_WRITE(io_base, SRGR1, reg_cache->srgr1 = config->srgr1); > + OMAP_MCBSP_WRITE(io_base, MCR2, reg_cache->mcr2 = config->mcr2); > + OMAP_MCBSP_WRITE(io_base, MCR1, reg_cache->mcr1 = config->mcr1); > + OMAP_MCBSP_WRITE(io_base, PCR0, reg_cache->pcr0 = config->pcr0); > if (cpu_is_omap2430() || cpu_is_omap34xx()) { > - OMAP_MCBSP_WRITE(io_base, XCCR, config->xccr); > - OMAP_MCBSP_WRITE(io_base, RCCR, config->rccr); > + OMAP_MCBSP_WRITE(io_base, XCCR, reg_cache->xccr = config->xccr); > + OMAP_MCBSP_WRITE(io_base, RCCR, reg_cache->rccr = config->rccr); > } > } > EXPORT_SYMBOL(omap_mcbsp_config); > @@ -232,6 +236,7 @@ EXPORT_SYMBOL(omap_mcbsp_set_io_type); > int omap_mcbsp_request(unsigned int id) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > int err; > > if (!omap_mcbsp_check_valid_id(id)) { > @@ -239,6 +244,7 @@ int omap_mcbsp_request(unsigned int id) > return -ENODEV; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > > spin_lock(&mcbsp->lock); > if (!mcbsp->free) { > @@ -261,8 +267,8 @@ int omap_mcbsp_request(unsigned int id) > * Make sure that transmitter, receiver and sample-rate generator are > * not running before activating IRQs. > */ > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, 0); > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, 0); > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, reg_cache->spcr1 = 0); > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, reg_cache->spcr2 = 0); > > if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) { > /* We need to get IRQs here */ > @@ -335,42 +341,38 @@ EXPORT_SYMBOL(omap_mcbsp_free); > void omap_mcbsp_start(unsigned int id, int tx, int rx) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > int idle; > - u16 w; > > if (!omap_mcbsp_check_valid_id(id)) { > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > return; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > > - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7; > - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7; > + mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7; > + mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7; > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > if (idle) { > /* Start the sample generator */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 6)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 6)); > } > > /* Enable transmitter and receiver */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (tx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (tx & 1)); > > - w = OMAP_MCBSP_READ(io_base, SPCR1); > - OMAP_MCBSP_WRITE(io_base, SPCR1, w | (rx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 |= (rx & 1)); > > udelay(100); > > if (idle) { > /* Start frame sync */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 7)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 7)); > } > > /* Dump McBSP Regs */ > @@ -381,9 +383,9 @@ EXPORT_SYMBOL(omap_mcbsp_start); > void omap_mcbsp_stop(unsigned int id, int tx, int rx) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > int idle; > - u16 w; > > if (!omap_mcbsp_check_valid_id(id)) { > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > @@ -391,23 +393,20 @@ void omap_mcbsp_stop(unsigned int id, in > } > > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > > /* Reset transmitter */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(tx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(tx & 1)); > > /* Reset receiver */ > - w = OMAP_MCBSP_READ(io_base, SPCR1); > - OMAP_MCBSP_WRITE(io_base, SPCR1, w & ~(rx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 &= ~(rx & 1)); > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > if (idle) { > /* Reset the sample rate generator */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(1 << 6)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(1 << 6)); > } > } > EXPORT_SYMBOL(omap_mcbsp_stop); > @@ -557,6 +556,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word); > int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > omap_mcbsp_word_length tx_word_length; > omap_mcbsp_word_length rx_word_length; > @@ -567,6 +567,7 @@ int omap_mcbsp_spi_master_xmit_word_poll > return -ENODEV; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > tx_word_length = mcbsp->tx_word_length; > rx_word_length = mcbsp->rx_word_length; > @@ -580,9 +581,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > if (attempts++ > 1000) { > /* We must reset the transmitter */ > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 &= (~XRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 |= XRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d transmitter not " > "ready\n", mcbsp->id); > @@ -601,9 +604,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > if (attempts++ > 1000) { > /* We must reset the receiver */ > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 &= (~RRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 |= RRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d receiver not " > "ready\n", mcbsp->id); > @@ -623,6 +628,7 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit > int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > u32 clock_word = 0; > void __iomem *io_base; > omap_mcbsp_word_length tx_word_length; > @@ -635,6 +641,7 @@ int omap_mcbsp_spi_master_recv_word_poll > } > > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > > tx_word_length = mcbsp->tx_word_length; > @@ -649,9 +656,11 @@ int omap_mcbsp_spi_master_recv_word_poll > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > if (attempts++ > 1000) { > /* We must reset the transmitter */ > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 &= (~XRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 |= XRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d transmitter not " > "ready\n", mcbsp->id); > @@ -670,9 +679,11 @@ int omap_mcbsp_spi_master_recv_word_poll > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > if (attempts++ > 1000) { > /* We must reset the receiver */ > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 &= (~RRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 |= RRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d receiver not " > "ready\n", mcbsp->id); > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-08-12 10:39 [RFC][PATCH] ARM: OMAP: McBSP: Use register cache Janusz Krzysztofik 2009-08-28 11:26 ` Janusz Krzysztofik @ 2009-11-14 2:06 ` Janusz Krzysztofik 2009-11-23 20:42 ` Janusz Krzysztofik 1 sibling, 1 reply; 15+ messages in thread From: Janusz Krzysztofik @ 2009-11-14 2:06 UTC (permalink / raw) To: Tony Lindgren, Jarkko Nikula, Samuel Ortiz Cc: Peter Ujfalusi, alsa-devel@alsa-project.org, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Hi, This patch got outdated and does not apply any more, but the idea still hangs in my queue. I am not sure why the patch has never been acknowledged nor rejected. Maybe it just got missed? Could you please take a position on this idea? Thanks, Janusz http://www.spinics.net/lists/linux-omap/msg16720.html Wednesday 12 August 2009 12:39:25 Janusz Krzysztofik wrote: > Change the way McBSP registers are updated: use cached values instead of > relying upon those read back from the device. > > With this patch, I have finally managed to get rid of all random > playback/recording hangups on my OMAP1510 based Amstrad Delta (buggy?) > hardware. Before that, I could suspect that values read back from McBSP > registers before updating them happened to be errornous. > > I think there is one important point that makes this patch worth of > applying, apart from my hardware quality. With the current code, if it ever > happens to any machine, no matter if OMAP1510 or newer, to read incorrect > value from a McBSP register, this wrong value will get written back without > any checking. That can lead to hardware damage if, for example, an input > pin is turned into output as a result. > > Created against linux-2.6.31-rc5 > > Tested on Amstrad Delta > > Signed-off-by: Janusz Krzysztofik > > --- > --- > linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h.orig 2009-08-11 > 23:43:25.000000000 +0200 +++ > linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h 2009-08-11 > 23:45:46.000000000 +0200 @@ -377,6 +377,8 @@ struct omap_mcbsp { > struct omap_mcbsp_platform_data *pdata; > struct clk *iclk; > struct clk *fclk; > + > + struct omap_mcbsp_reg_cfg reg_cache; > }; > extern struct omap_mcbsp **mcbsp_ptr; > extern int omap_mcbsp_count; > --- linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c.orig 2009-08-11 > 23:43:25.000000000 +0200 +++ > linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c 2009-08-11 23:45:35.000000000 > +0200 @@ -91,6 +91,7 @@ static void omap_mcbsp_dump_reg(u8 id) > static irqreturn_t omap_mcbsp_tx_irq_handler(int irq, void *dev_id) > { > struct omap_mcbsp *mcbsp_tx = dev_id; > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_tx->reg_cache; > u16 irqst_spcr2; > > irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); > @@ -101,7 +102,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > irqst_spcr2); > /* Writing zero to XSYNC_ERR clears the IRQ */ > OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, > - irqst_spcr2 & ~(XSYNC_ERR)); > + reg_cache->spcr2 &= ~(XSYNC_ERR)); > } else { > complete(&mcbsp_tx->tx_irq_completion); > } > @@ -112,6 +113,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > static irqreturn_t omap_mcbsp_rx_irq_handler(int irq, void *dev_id) > { > struct omap_mcbsp *mcbsp_rx = dev_id; > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_rx->reg_cache; > u16 irqst_spcr1; > > irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx->io_base, SPCR1); > @@ -122,7 +124,7 @@ static irqreturn_t omap_mcbsp_rx_irq_han > irqst_spcr1); > /* Writing zero to RSYNC_ERR clears the IRQ */ > OMAP_MCBSP_WRITE(mcbsp_rx->io_base, SPCR1, > - irqst_spcr1 & ~(RSYNC_ERR)); > + reg_cache->spcr1 &= ~(RSYNC_ERR)); > } else { > complete(&mcbsp_rx->tx_irq_completion); > } > @@ -167,6 +169,7 @@ static void omap_mcbsp_rx_dma_callback(i > void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg > *config) { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > > if (!omap_mcbsp_check_valid_id(id)) { > @@ -174,26 +177,27 @@ void omap_mcbsp_config(unsigned int id, > return; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > > io_base = mcbsp->io_base; > dev_dbg(mcbsp->dev, "Configuring McBSP%d phys_base: 0x%08lx\n", > mcbsp->id, mcbsp->phys_base); > > /* We write the given config */ > - OMAP_MCBSP_WRITE(io_base, SPCR2, config->spcr2); > - OMAP_MCBSP_WRITE(io_base, SPCR1, config->spcr1); > - OMAP_MCBSP_WRITE(io_base, RCR2, config->rcr2); > - OMAP_MCBSP_WRITE(io_base, RCR1, config->rcr1); > - OMAP_MCBSP_WRITE(io_base, XCR2, config->xcr2); > - OMAP_MCBSP_WRITE(io_base, XCR1, config->xcr1); > - OMAP_MCBSP_WRITE(io_base, SRGR2, config->srgr2); > - OMAP_MCBSP_WRITE(io_base, SRGR1, config->srgr1); > - OMAP_MCBSP_WRITE(io_base, MCR2, config->mcr2); > - OMAP_MCBSP_WRITE(io_base, MCR1, config->mcr1); > - OMAP_MCBSP_WRITE(io_base, PCR0, config->pcr0); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = config->spcr2); > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 = config->spcr1); > + OMAP_MCBSP_WRITE(io_base, RCR2, reg_cache->rcr2 = config->rcr2); > + OMAP_MCBSP_WRITE(io_base, RCR1, reg_cache->rcr1 = config->rcr1); > + OMAP_MCBSP_WRITE(io_base, XCR2, reg_cache->xcr2 = config->xcr2); > + OMAP_MCBSP_WRITE(io_base, XCR1, reg_cache->xcr1 = config->xcr1); > + OMAP_MCBSP_WRITE(io_base, SRGR2, reg_cache->srgr2 = config->srgr2); > + OMAP_MCBSP_WRITE(io_base, SRGR1, reg_cache->srgr1 = config->srgr1); > + OMAP_MCBSP_WRITE(io_base, MCR2, reg_cache->mcr2 = config->mcr2); > + OMAP_MCBSP_WRITE(io_base, MCR1, reg_cache->mcr1 = config->mcr1); > + OMAP_MCBSP_WRITE(io_base, PCR0, reg_cache->pcr0 = config->pcr0); > if (cpu_is_omap2430() || cpu_is_omap34xx()) { > - OMAP_MCBSP_WRITE(io_base, XCCR, config->xccr); > - OMAP_MCBSP_WRITE(io_base, RCCR, config->rccr); > + OMAP_MCBSP_WRITE(io_base, XCCR, reg_cache->xccr = config->xccr); > + OMAP_MCBSP_WRITE(io_base, RCCR, reg_cache->rccr = config->rccr); > } > } > EXPORT_SYMBOL(omap_mcbsp_config); > @@ -232,6 +236,7 @@ EXPORT_SYMBOL(omap_mcbsp_set_io_type); > int omap_mcbsp_request(unsigned int id) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > int err; > > if (!omap_mcbsp_check_valid_id(id)) { > @@ -239,6 +244,7 @@ int omap_mcbsp_request(unsigned int id) > return -ENODEV; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > > spin_lock(&mcbsp->lock); > if (!mcbsp->free) { > @@ -261,8 +267,8 @@ int omap_mcbsp_request(unsigned int id) > * Make sure that transmitter, receiver and sample-rate generator are > * not running before activating IRQs. > */ > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, 0); > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, 0); > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, reg_cache->spcr1 = 0); > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, reg_cache->spcr2 = 0); > > if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) { > /* We need to get IRQs here */ > @@ -335,42 +341,38 @@ EXPORT_SYMBOL(omap_mcbsp_free); > void omap_mcbsp_start(unsigned int id, int tx, int rx) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > int idle; > - u16 w; > > if (!omap_mcbsp_check_valid_id(id)) { > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > return; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > > - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7; > - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7; > + mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7; > + mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7; > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > if (idle) { > /* Start the sample generator */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 6)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 6)); > } > > /* Enable transmitter and receiver */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (tx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (tx & 1)); > > - w = OMAP_MCBSP_READ(io_base, SPCR1); > - OMAP_MCBSP_WRITE(io_base, SPCR1, w | (rx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 |= (rx & 1)); > > udelay(100); > > if (idle) { > /* Start frame sync */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 7)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 7)); > } > > /* Dump McBSP Regs */ > @@ -381,9 +383,9 @@ EXPORT_SYMBOL(omap_mcbsp_start); > void omap_mcbsp_stop(unsigned int id, int tx, int rx) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > int idle; > - u16 w; > > if (!omap_mcbsp_check_valid_id(id)) { > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > @@ -391,23 +393,20 @@ void omap_mcbsp_stop(unsigned int id, in > } > > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > > /* Reset transmitter */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(tx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(tx & 1)); > > /* Reset receiver */ > - w = OMAP_MCBSP_READ(io_base, SPCR1); > - OMAP_MCBSP_WRITE(io_base, SPCR1, w & ~(rx & 1)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 &= ~(rx & 1)); > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > if (idle) { > /* Reset the sample rate generator */ > - w = OMAP_MCBSP_READ(io_base, SPCR2); > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(1 << 6)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(1 << 6)); > } > } > EXPORT_SYMBOL(omap_mcbsp_stop); > @@ -557,6 +556,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word); > int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > void __iomem *io_base; > omap_mcbsp_word_length tx_word_length; > omap_mcbsp_word_length rx_word_length; > @@ -567,6 +567,7 @@ int omap_mcbsp_spi_master_xmit_word_poll > return -ENODEV; > } > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > tx_word_length = mcbsp->tx_word_length; > rx_word_length = mcbsp->rx_word_length; > @@ -580,9 +581,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > if (attempts++ > 1000) { > /* We must reset the transmitter */ > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 &= (~XRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 |= XRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d transmitter not " > "ready\n", mcbsp->id); > @@ -601,9 +604,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > if (attempts++ > 1000) { > /* We must reset the receiver */ > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 &= (~RRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 |= RRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d receiver not " > "ready\n", mcbsp->id); > @@ -623,6 +628,7 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit > int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word) > { > struct omap_mcbsp *mcbsp; > + struct omap_mcbsp_reg_cfg *reg_cache; > u32 clock_word = 0; > void __iomem *io_base; > omap_mcbsp_word_length tx_word_length; > @@ -635,6 +641,7 @@ int omap_mcbsp_spi_master_recv_word_poll > } > > mcbsp = id_to_mcbsp_ptr(id); > + reg_cache = &mcbsp->reg_cache; > io_base = mcbsp->io_base; > > tx_word_length = mcbsp->tx_word_length; > @@ -649,9 +656,11 @@ int omap_mcbsp_spi_master_recv_word_poll > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > if (attempts++ > 1000) { > /* We must reset the transmitter */ > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 &= (~XRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > + OMAP_MCBSP_WRITE(io_base, SPCR2, > + reg_cache->spcr2 |= XRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d transmitter not " > "ready\n", mcbsp->id); > @@ -670,9 +679,11 @@ int omap_mcbsp_spi_master_recv_word_poll > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > if (attempts++ > 1000) { > /* We must reset the receiver */ > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 &= (~RRST)); > udelay(10); > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > + OMAP_MCBSP_WRITE(io_base, SPCR1, > + reg_cache->spcr1 |= RRST); > udelay(10); > dev_err(mcbsp->dev, "McBSP%d receiver not " > "ready\n", mcbsp->id); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-11-14 2:06 ` Janusz Krzysztofik @ 2009-11-23 20:42 ` Janusz Krzysztofik 2009-11-23 23:53 ` Tony Lindgren 0 siblings, 1 reply; 15+ messages in thread From: Janusz Krzysztofik @ 2009-11-23 20:42 UTC (permalink / raw) To: Tony Lindgren; +Cc: linux-omap@vger.kernel.org, Jarkko Nikula (drop all except Tony, Jarkko and linux-omap) Saturday 14 November 2009 03:06:39 Janusz Krzysztofik napisał(a): > Hi, > > This patch got outdated and does not apply any more, but the idea still > hangs in my queue. I am not sure why the patch has never been acknowledged > nor rejected. Maybe it just got missed? Tony, I have just found this patch, initially submitted on 2009-08-12, archived in linux-omap patchwork with State: Awaiting Upstream. What does it mean? Am I supposed to do something about it? After that many weeks of no negative comments nor requests for changes, and taking into account what Jarkko said lately in a different thread[*] about the McBSP register cache concept, can I assume that it has been generally accepted? Is it worth of refreshing against current linux-omap? Thanks, Janusz > Could you please take a position on this idea? > > Thanks, > Janusz > > http://www.spinics.net/lists/linux-omap/msg16720.html [*] http://www.spinics.net/lists/linux-omap/msg20597.html > Wednesday 12 August 2009 12:39:25 Janusz Krzysztofik wrote: > > Change the way McBSP registers are updated: use cached values instead of > > relying upon those read back from the device. > > > > With this patch, I have finally managed to get rid of all random > > playback/recording hangups on my OMAP1510 based Amstrad Delta (buggy?) > > hardware. Before that, I could suspect that values read back from McBSP > > registers before updating them happened to be errornous. > > > > I think there is one important point that makes this patch worth of > > applying, apart from my hardware quality. With the current code, if it > > ever happens to any machine, no matter if OMAP1510 or newer, to read > > incorrect value from a McBSP register, this wrong value will get written > > back without any checking. That can lead to hardware damage if, for > > example, an input pin is turned into output as a result. > > > > Created against linux-2.6.31-rc5 > > > > Tested on Amstrad Delta > > > > Signed-off-by: Janusz Krzysztofik > > > > --- > > --- > > linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h.orig 2009-08-11 > > 23:43:25.000000000 +0200 +++ > > linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h 2009-08-11 > > 23:45:46.000000000 +0200 @@ -377,6 +377,8 @@ struct omap_mcbsp { > > struct omap_mcbsp_platform_data *pdata; > > struct clk *iclk; > > struct clk *fclk; > > + > > + struct omap_mcbsp_reg_cfg reg_cache; > > }; > > extern struct omap_mcbsp **mcbsp_ptr; > > extern int omap_mcbsp_count; > > --- linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c.orig 2009-08-11 > > 23:43:25.000000000 +0200 +++ > > linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c 2009-08-11 23:45:35.000000000 > > +0200 @@ -91,6 +91,7 @@ static void omap_mcbsp_dump_reg(u8 id) > > static irqreturn_t omap_mcbsp_tx_irq_handler(int irq, void *dev_id) > > { > > struct omap_mcbsp *mcbsp_tx = dev_id; > > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_tx->reg_cache; > > u16 irqst_spcr2; > > > > irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); > > @@ -101,7 +102,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > > irqst_spcr2); > > /* Writing zero to XSYNC_ERR clears the IRQ */ > > OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, > > - irqst_spcr2 & ~(XSYNC_ERR)); > > + reg_cache->spcr2 &= ~(XSYNC_ERR)); > > } else { > > complete(&mcbsp_tx->tx_irq_completion); > > } > > @@ -112,6 +113,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > > static irqreturn_t omap_mcbsp_rx_irq_handler(int irq, void *dev_id) > > { > > struct omap_mcbsp *mcbsp_rx = dev_id; > > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_rx->reg_cache; > > u16 irqst_spcr1; > > > > irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx->io_base, SPCR1); > > @@ -122,7 +124,7 @@ static irqreturn_t omap_mcbsp_rx_irq_han > > irqst_spcr1); > > /* Writing zero to RSYNC_ERR clears the IRQ */ > > OMAP_MCBSP_WRITE(mcbsp_rx->io_base, SPCR1, > > - irqst_spcr1 & ~(RSYNC_ERR)); > > + reg_cache->spcr1 &= ~(RSYNC_ERR)); > > } else { > > complete(&mcbsp_rx->tx_irq_completion); > > } > > @@ -167,6 +169,7 @@ static void omap_mcbsp_rx_dma_callback(i > > void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg > > *config) { > > struct omap_mcbsp *mcbsp; > > + struct omap_mcbsp_reg_cfg *reg_cache; > > void __iomem *io_base; > > > > if (!omap_mcbsp_check_valid_id(id)) { > > @@ -174,26 +177,27 @@ void omap_mcbsp_config(unsigned int id, > > return; > > } > > mcbsp = id_to_mcbsp_ptr(id); > > + reg_cache = &mcbsp->reg_cache; > > > > io_base = mcbsp->io_base; > > dev_dbg(mcbsp->dev, "Configuring McBSP%d phys_base: 0x%08lx\n", > > mcbsp->id, mcbsp->phys_base); > > > > /* We write the given config */ > > - OMAP_MCBSP_WRITE(io_base, SPCR2, config->spcr2); > > - OMAP_MCBSP_WRITE(io_base, SPCR1, config->spcr1); > > - OMAP_MCBSP_WRITE(io_base, RCR2, config->rcr2); > > - OMAP_MCBSP_WRITE(io_base, RCR1, config->rcr1); > > - OMAP_MCBSP_WRITE(io_base, XCR2, config->xcr2); > > - OMAP_MCBSP_WRITE(io_base, XCR1, config->xcr1); > > - OMAP_MCBSP_WRITE(io_base, SRGR2, config->srgr2); > > - OMAP_MCBSP_WRITE(io_base, SRGR1, config->srgr1); > > - OMAP_MCBSP_WRITE(io_base, MCR2, config->mcr2); > > - OMAP_MCBSP_WRITE(io_base, MCR1, config->mcr1); > > - OMAP_MCBSP_WRITE(io_base, PCR0, config->pcr0); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = config->spcr2); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 = config->spcr1); > > + OMAP_MCBSP_WRITE(io_base, RCR2, reg_cache->rcr2 = config->rcr2); > > + OMAP_MCBSP_WRITE(io_base, RCR1, reg_cache->rcr1 = config->rcr1); > > + OMAP_MCBSP_WRITE(io_base, XCR2, reg_cache->xcr2 = config->xcr2); > > + OMAP_MCBSP_WRITE(io_base, XCR1, reg_cache->xcr1 = config->xcr1); > > + OMAP_MCBSP_WRITE(io_base, SRGR2, reg_cache->srgr2 = config->srgr2); > > + OMAP_MCBSP_WRITE(io_base, SRGR1, reg_cache->srgr1 = config->srgr1); > > + OMAP_MCBSP_WRITE(io_base, MCR2, reg_cache->mcr2 = config->mcr2); > > + OMAP_MCBSP_WRITE(io_base, MCR1, reg_cache->mcr1 = config->mcr1); > > + OMAP_MCBSP_WRITE(io_base, PCR0, reg_cache->pcr0 = config->pcr0); > > if (cpu_is_omap2430() || cpu_is_omap34xx()) { > > - OMAP_MCBSP_WRITE(io_base, XCCR, config->xccr); > > - OMAP_MCBSP_WRITE(io_base, RCCR, config->rccr); > > + OMAP_MCBSP_WRITE(io_base, XCCR, reg_cache->xccr = config->xccr); > > + OMAP_MCBSP_WRITE(io_base, RCCR, reg_cache->rccr = config->rccr); > > } > > } > > EXPORT_SYMBOL(omap_mcbsp_config); > > @@ -232,6 +236,7 @@ EXPORT_SYMBOL(omap_mcbsp_set_io_type); > > int omap_mcbsp_request(unsigned int id) > > { > > struct omap_mcbsp *mcbsp; > > + struct omap_mcbsp_reg_cfg *reg_cache; > > int err; > > > > if (!omap_mcbsp_check_valid_id(id)) { > > @@ -239,6 +244,7 @@ int omap_mcbsp_request(unsigned int id) > > return -ENODEV; > > } > > mcbsp = id_to_mcbsp_ptr(id); > > + reg_cache = &mcbsp->reg_cache; > > > > spin_lock(&mcbsp->lock); > > if (!mcbsp->free) { > > @@ -261,8 +267,8 @@ int omap_mcbsp_request(unsigned int id) > > * Make sure that transmitter, receiver and sample-rate generator are > > * not running before activating IRQs. > > */ > > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, 0); > > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, 0); > > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, reg_cache->spcr1 = 0); > > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, reg_cache->spcr2 = 0); > > > > if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) { > > /* We need to get IRQs here */ > > @@ -335,42 +341,38 @@ EXPORT_SYMBOL(omap_mcbsp_free); > > void omap_mcbsp_start(unsigned int id, int tx, int rx) > > { > > struct omap_mcbsp *mcbsp; > > + struct omap_mcbsp_reg_cfg *reg_cache; > > void __iomem *io_base; > > int idle; > > - u16 w; > > > > if (!omap_mcbsp_check_valid_id(id)) { > > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > > return; > > } > > mcbsp = id_to_mcbsp_ptr(id); > > + reg_cache = &mcbsp->reg_cache; > > io_base = mcbsp->io_base; > > > > - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7; > > - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7; > > + mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7; > > + mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7; > > > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > > > if (idle) { > > /* Start the sample generator */ > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 6)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 6)); > > } > > > > /* Enable transmitter and receiver */ > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (tx & 1)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (tx & 1)); > > > > - w = OMAP_MCBSP_READ(io_base, SPCR1); > > - OMAP_MCBSP_WRITE(io_base, SPCR1, w | (rx & 1)); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 |= (rx & 1)); > > > > udelay(100); > > > > if (idle) { > > /* Start frame sync */ > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 7)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 7)); > > } > > > > /* Dump McBSP Regs */ > > @@ -381,9 +383,9 @@ EXPORT_SYMBOL(omap_mcbsp_start); > > void omap_mcbsp_stop(unsigned int id, int tx, int rx) > > { > > struct omap_mcbsp *mcbsp; > > + struct omap_mcbsp_reg_cfg *reg_cache; > > void __iomem *io_base; > > int idle; > > - u16 w; > > > > if (!omap_mcbsp_check_valid_id(id)) { > > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > > @@ -391,23 +393,20 @@ void omap_mcbsp_stop(unsigned int id, in > > } > > > > mcbsp = id_to_mcbsp_ptr(id); > > + reg_cache = &mcbsp->reg_cache; > > io_base = mcbsp->io_base; > > > > /* Reset transmitter */ > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(tx & 1)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(tx & 1)); > > > > /* Reset receiver */ > > - w = OMAP_MCBSP_READ(io_base, SPCR1); > > - OMAP_MCBSP_WRITE(io_base, SPCR1, w & ~(rx & 1)); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 &= ~(rx & 1)); > > > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > > > if (idle) { > > /* Reset the sample rate generator */ > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(1 << 6)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(1 << 6)); > > } > > } > > EXPORT_SYMBOL(omap_mcbsp_stop); > > @@ -557,6 +556,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word); > > int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word) > > { > > struct omap_mcbsp *mcbsp; > > + struct omap_mcbsp_reg_cfg *reg_cache; > > void __iomem *io_base; > > omap_mcbsp_word_length tx_word_length; > > omap_mcbsp_word_length rx_word_length; > > @@ -567,6 +567,7 @@ int omap_mcbsp_spi_master_xmit_word_poll > > return -ENODEV; > > } > > mcbsp = id_to_mcbsp_ptr(id); > > + reg_cache = &mcbsp->reg_cache; > > io_base = mcbsp->io_base; > > tx_word_length = mcbsp->tx_word_length; > > rx_word_length = mcbsp->rx_word_length; > > @@ -580,9 +581,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > > if (attempts++ > 1000) { > > /* We must reset the transmitter */ > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > + reg_cache->spcr2 &= (~XRST)); > > udelay(10); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > + reg_cache->spcr2 |= XRST); > > udelay(10); > > dev_err(mcbsp->dev, "McBSP%d transmitter not " > > "ready\n", mcbsp->id); > > @@ -601,9 +604,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > > if (attempts++ > 1000) { > > /* We must reset the receiver */ > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > + reg_cache->spcr1 &= (~RRST)); > > udelay(10); > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > + reg_cache->spcr1 |= RRST); > > udelay(10); > > dev_err(mcbsp->dev, "McBSP%d receiver not " > > "ready\n", mcbsp->id); > > @@ -623,6 +628,7 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit > > int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word) > > { > > struct omap_mcbsp *mcbsp; > > + struct omap_mcbsp_reg_cfg *reg_cache; > > u32 clock_word = 0; > > void __iomem *io_base; > > omap_mcbsp_word_length tx_word_length; > > @@ -635,6 +641,7 @@ int omap_mcbsp_spi_master_recv_word_poll > > } > > > > mcbsp = id_to_mcbsp_ptr(id); > > + reg_cache = &mcbsp->reg_cache; > > io_base = mcbsp->io_base; > > > > tx_word_length = mcbsp->tx_word_length; > > @@ -649,9 +656,11 @@ int omap_mcbsp_spi_master_recv_word_poll > > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > > if (attempts++ > 1000) { > > /* We must reset the transmitter */ > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > + reg_cache->spcr2 &= (~XRST)); > > udelay(10); > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > + reg_cache->spcr2 |= XRST); > > udelay(10); > > dev_err(mcbsp->dev, "McBSP%d transmitter not " > > "ready\n", mcbsp->id); > > @@ -670,9 +679,11 @@ int omap_mcbsp_spi_master_recv_word_poll > > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > > if (attempts++ > 1000) { > > /* We must reset the receiver */ > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > + reg_cache->spcr1 &= (~RRST)); > > udelay(10); > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > + reg_cache->spcr1 |= RRST); > > udelay(10); > > dev_err(mcbsp->dev, "McBSP%d receiver not " > > "ready\n", mcbsp->id); -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-11-23 20:42 ` Janusz Krzysztofik @ 2009-11-23 23:53 ` Tony Lindgren 2009-11-24 7:59 ` Jarkko Nikula 0 siblings, 1 reply; 15+ messages in thread From: Tony Lindgren @ 2009-11-23 23:53 UTC (permalink / raw) To: Janusz Krzysztofik; +Cc: linux-omap@vger.kernel.org, Jarkko Nikula * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [091123 12:42]: > (drop all except Tony, Jarkko and linux-omap) > > Saturday 14 November 2009 03:06:39 Janusz Krzysztofik napisał(a): > > Hi, > > > > This patch got outdated and does not apply any more, but the idea still > > hangs in my queue. I am not sure why the patch has never been acknowledged > > nor rejected. Maybe it just got missed? > > Tony, > > I have just found this patch, initially submitted on 2009-08-12, archived in > linux-omap patchwork with State: Awaiting Upstream. What does it mean? Am I > supposed to do something about it? After that many weeks of no negative > comments nor requests for changes, and taking into account what Jarkko said > lately in a different thread[*] about the McBSP register cache concept, can I > assume that it has been generally accepted? Is it worth of refreshing against > current linux-omap? I'll just mark patches "Awaiting upstream" if I feel like somebody else should ack or merge it. If there are obvious changes needed, I'll mark it "Changes requested". So for the patch, please refresh. Then let's apply it assuming Jarkko acks it. Regards, Tony > Thanks, > Janusz > > > Could you please take a position on this idea? > > > > Thanks, > > Janusz > > > > http://www.spinics.net/lists/linux-omap/msg16720.html > > [*] http://www.spinics.net/lists/linux-omap/msg20597.html > > > Wednesday 12 August 2009 12:39:25 Janusz Krzysztofik wrote: > > > Change the way McBSP registers are updated: use cached values instead of > > > relying upon those read back from the device. > > > > > > With this patch, I have finally managed to get rid of all random > > > playback/recording hangups on my OMAP1510 based Amstrad Delta (buggy?) > > > hardware. Before that, I could suspect that values read back from McBSP > > > registers before updating them happened to be errornous. > > > > > > I think there is one important point that makes this patch worth of > > > applying, apart from my hardware quality. With the current code, if it > > > ever happens to any machine, no matter if OMAP1510 or newer, to read > > > incorrect value from a McBSP register, this wrong value will get written > > > back without any checking. That can lead to hardware damage if, for > > > example, an input pin is turned into output as a result. > > > > > > Created against linux-2.6.31-rc5 > > > > > > Tested on Amstrad Delta > > > > > > Signed-off-by: Janusz Krzysztofik > > > > > > --- > > > --- > > > linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h.orig 2009-08-11 > > > 23:43:25.000000000 +0200 +++ > > > linux-2.6.31-rc5/arch/arm/plat-omap/include/mach/mcbsp.h 2009-08-11 > > > 23:45:46.000000000 +0200 @@ -377,6 +377,8 @@ struct omap_mcbsp { > > > struct omap_mcbsp_platform_data *pdata; > > > struct clk *iclk; > > > struct clk *fclk; > > > + > > > + struct omap_mcbsp_reg_cfg reg_cache; > > > }; > > > extern struct omap_mcbsp **mcbsp_ptr; > > > extern int omap_mcbsp_count; > > > --- linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c.orig 2009-08-11 > > > 23:43:25.000000000 +0200 +++ > > > linux-2.6.31-rc5/arch/arm/plat-omap/mcbsp.c 2009-08-11 23:45:35.000000000 > > > +0200 @@ -91,6 +91,7 @@ static void omap_mcbsp_dump_reg(u8 id) > > > static irqreturn_t omap_mcbsp_tx_irq_handler(int irq, void *dev_id) > > > { > > > struct omap_mcbsp *mcbsp_tx = dev_id; > > > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_tx->reg_cache; > > > u16 irqst_spcr2; > > > > > > irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); > > > @@ -101,7 +102,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > > > irqst_spcr2); > > > /* Writing zero to XSYNC_ERR clears the IRQ */ > > > OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, > > > - irqst_spcr2 & ~(XSYNC_ERR)); > > > + reg_cache->spcr2 &= ~(XSYNC_ERR)); > > > } else { > > > complete(&mcbsp_tx->tx_irq_completion); > > > } > > > @@ -112,6 +113,7 @@ static irqreturn_t omap_mcbsp_tx_irq_han > > > static irqreturn_t omap_mcbsp_rx_irq_handler(int irq, void *dev_id) > > > { > > > struct omap_mcbsp *mcbsp_rx = dev_id; > > > + struct omap_mcbsp_reg_cfg *reg_cache = &mcbsp_rx->reg_cache; > > > u16 irqst_spcr1; > > > > > > irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx->io_base, SPCR1); > > > @@ -122,7 +124,7 @@ static irqreturn_t omap_mcbsp_rx_irq_han > > > irqst_spcr1); > > > /* Writing zero to RSYNC_ERR clears the IRQ */ > > > OMAP_MCBSP_WRITE(mcbsp_rx->io_base, SPCR1, > > > - irqst_spcr1 & ~(RSYNC_ERR)); > > > + reg_cache->spcr1 &= ~(RSYNC_ERR)); > > > } else { > > > complete(&mcbsp_rx->tx_irq_completion); > > > } > > > @@ -167,6 +169,7 @@ static void omap_mcbsp_rx_dma_callback(i > > > void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg > > > *config) { > > > struct omap_mcbsp *mcbsp; > > > + struct omap_mcbsp_reg_cfg *reg_cache; > > > void __iomem *io_base; > > > > > > if (!omap_mcbsp_check_valid_id(id)) { > > > @@ -174,26 +177,27 @@ void omap_mcbsp_config(unsigned int id, > > > return; > > > } > > > mcbsp = id_to_mcbsp_ptr(id); > > > + reg_cache = &mcbsp->reg_cache; > > > > > > io_base = mcbsp->io_base; > > > dev_dbg(mcbsp->dev, "Configuring McBSP%d phys_base: 0x%08lx\n", > > > mcbsp->id, mcbsp->phys_base); > > > > > > /* We write the given config */ > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, config->spcr2); > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, config->spcr1); > > > - OMAP_MCBSP_WRITE(io_base, RCR2, config->rcr2); > > > - OMAP_MCBSP_WRITE(io_base, RCR1, config->rcr1); > > > - OMAP_MCBSP_WRITE(io_base, XCR2, config->xcr2); > > > - OMAP_MCBSP_WRITE(io_base, XCR1, config->xcr1); > > > - OMAP_MCBSP_WRITE(io_base, SRGR2, config->srgr2); > > > - OMAP_MCBSP_WRITE(io_base, SRGR1, config->srgr1); > > > - OMAP_MCBSP_WRITE(io_base, MCR2, config->mcr2); > > > - OMAP_MCBSP_WRITE(io_base, MCR1, config->mcr1); > > > - OMAP_MCBSP_WRITE(io_base, PCR0, config->pcr0); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = config->spcr2); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 = config->spcr1); > > > + OMAP_MCBSP_WRITE(io_base, RCR2, reg_cache->rcr2 = config->rcr2); > > > + OMAP_MCBSP_WRITE(io_base, RCR1, reg_cache->rcr1 = config->rcr1); > > > + OMAP_MCBSP_WRITE(io_base, XCR2, reg_cache->xcr2 = config->xcr2); > > > + OMAP_MCBSP_WRITE(io_base, XCR1, reg_cache->xcr1 = config->xcr1); > > > + OMAP_MCBSP_WRITE(io_base, SRGR2, reg_cache->srgr2 = config->srgr2); > > > + OMAP_MCBSP_WRITE(io_base, SRGR1, reg_cache->srgr1 = config->srgr1); > > > + OMAP_MCBSP_WRITE(io_base, MCR2, reg_cache->mcr2 = config->mcr2); > > > + OMAP_MCBSP_WRITE(io_base, MCR1, reg_cache->mcr1 = config->mcr1); > > > + OMAP_MCBSP_WRITE(io_base, PCR0, reg_cache->pcr0 = config->pcr0); > > > if (cpu_is_omap2430() || cpu_is_omap34xx()) { > > > - OMAP_MCBSP_WRITE(io_base, XCCR, config->xccr); > > > - OMAP_MCBSP_WRITE(io_base, RCCR, config->rccr); > > > + OMAP_MCBSP_WRITE(io_base, XCCR, reg_cache->xccr = config->xccr); > > > + OMAP_MCBSP_WRITE(io_base, RCCR, reg_cache->rccr = config->rccr); > > > } > > > } > > > EXPORT_SYMBOL(omap_mcbsp_config); > > > @@ -232,6 +236,7 @@ EXPORT_SYMBOL(omap_mcbsp_set_io_type); > > > int omap_mcbsp_request(unsigned int id) > > > { > > > struct omap_mcbsp *mcbsp; > > > + struct omap_mcbsp_reg_cfg *reg_cache; > > > int err; > > > > > > if (!omap_mcbsp_check_valid_id(id)) { > > > @@ -239,6 +244,7 @@ int omap_mcbsp_request(unsigned int id) > > > return -ENODEV; > > > } > > > mcbsp = id_to_mcbsp_ptr(id); > > > + reg_cache = &mcbsp->reg_cache; > > > > > > spin_lock(&mcbsp->lock); > > > if (!mcbsp->free) { > > > @@ -261,8 +267,8 @@ int omap_mcbsp_request(unsigned int id) > > > * Make sure that transmitter, receiver and sample-rate generator are > > > * not running before activating IRQs. > > > */ > > > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, 0); > > > - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, 0); > > > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, reg_cache->spcr1 = 0); > > > + OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, reg_cache->spcr2 = 0); > > > > > > if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) { > > > /* We need to get IRQs here */ > > > @@ -335,42 +341,38 @@ EXPORT_SYMBOL(omap_mcbsp_free); > > > void omap_mcbsp_start(unsigned int id, int tx, int rx) > > > { > > > struct omap_mcbsp *mcbsp; > > > + struct omap_mcbsp_reg_cfg *reg_cache; > > > void __iomem *io_base; > > > int idle; > > > - u16 w; > > > > > > if (!omap_mcbsp_check_valid_id(id)) { > > > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > > > return; > > > } > > > mcbsp = id_to_mcbsp_ptr(id); > > > + reg_cache = &mcbsp->reg_cache; > > > io_base = mcbsp->io_base; > > > > > > - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7; > > > - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7; > > > + mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7; > > > + mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7; > > > > > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > > > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > > > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > > > > > if (idle) { > > > /* Start the sample generator */ > > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 6)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 6)); > > > } > > > > > > /* Enable transmitter and receiver */ > > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (tx & 1)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (tx & 1)); > > > > > > - w = OMAP_MCBSP_READ(io_base, SPCR1); > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, w | (rx & 1)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 |= (rx & 1)); > > > > > > udelay(100); > > > > > > if (idle) { > > > /* Start frame sync */ > > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 7)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 |= (1 << 7)); > > > } > > > > > > /* Dump McBSP Regs */ > > > @@ -381,9 +383,9 @@ EXPORT_SYMBOL(omap_mcbsp_start); > > > void omap_mcbsp_stop(unsigned int id, int tx, int rx) > > > { > > > struct omap_mcbsp *mcbsp; > > > + struct omap_mcbsp_reg_cfg *reg_cache; > > > void __iomem *io_base; > > > int idle; > > > - u16 w; > > > > > > if (!omap_mcbsp_check_valid_id(id)) { > > > printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); > > > @@ -391,23 +393,20 @@ void omap_mcbsp_stop(unsigned int id, in > > > } > > > > > > mcbsp = id_to_mcbsp_ptr(id); > > > + reg_cache = &mcbsp->reg_cache; > > > io_base = mcbsp->io_base; > > > > > > /* Reset transmitter */ > > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(tx & 1)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(tx & 1)); > > > > > > /* Reset receiver */ > > > - w = OMAP_MCBSP_READ(io_base, SPCR1); > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, w & ~(rx & 1)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 &= ~(rx & 1)); > > > > > > - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | > > > - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); > > > + idle = !((reg_cache->spcr2 | reg_cache->spcr1) & 1); > > > > > > if (idle) { > > > /* Reset the sample rate generator */ > > > - w = OMAP_MCBSP_READ(io_base, SPCR2); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(1 << 6)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 &= ~(1 << 6)); > > > } > > > } > > > EXPORT_SYMBOL(omap_mcbsp_stop); > > > @@ -557,6 +556,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word); > > > int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word) > > > { > > > struct omap_mcbsp *mcbsp; > > > + struct omap_mcbsp_reg_cfg *reg_cache; > > > void __iomem *io_base; > > > omap_mcbsp_word_length tx_word_length; > > > omap_mcbsp_word_length rx_word_length; > > > @@ -567,6 +567,7 @@ int omap_mcbsp_spi_master_xmit_word_poll > > > return -ENODEV; > > > } > > > mcbsp = id_to_mcbsp_ptr(id); > > > + reg_cache = &mcbsp->reg_cache; > > > io_base = mcbsp->io_base; > > > tx_word_length = mcbsp->tx_word_length; > > > rx_word_length = mcbsp->rx_word_length; > > > @@ -580,9 +581,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > > > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > > > if (attempts++ > 1000) { > > > /* We must reset the transmitter */ > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > > + reg_cache->spcr2 &= (~XRST)); > > > udelay(10); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > > + reg_cache->spcr2 |= XRST); > > > udelay(10); > > > dev_err(mcbsp->dev, "McBSP%d transmitter not " > > > "ready\n", mcbsp->id); > > > @@ -601,9 +604,11 @@ int omap_mcbsp_spi_master_xmit_word_poll > > > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > > > if (attempts++ > 1000) { > > > /* We must reset the receiver */ > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > > + reg_cache->spcr1 &= (~RRST)); > > > udelay(10); > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > > + reg_cache->spcr1 |= RRST); > > > udelay(10); > > > dev_err(mcbsp->dev, "McBSP%d receiver not " > > > "ready\n", mcbsp->id); > > > @@ -623,6 +628,7 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit > > > int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word) > > > { > > > struct omap_mcbsp *mcbsp; > > > + struct omap_mcbsp_reg_cfg *reg_cache; > > > u32 clock_word = 0; > > > void __iomem *io_base; > > > omap_mcbsp_word_length tx_word_length; > > > @@ -635,6 +641,7 @@ int omap_mcbsp_spi_master_recv_word_poll > > > } > > > > > > mcbsp = id_to_mcbsp_ptr(id); > > > + reg_cache = &mcbsp->reg_cache; > > > io_base = mcbsp->io_base; > > > > > > tx_word_length = mcbsp->tx_word_length; > > > @@ -649,9 +656,11 @@ int omap_mcbsp_spi_master_recv_word_poll > > > spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); > > > if (attempts++ > 1000) { > > > /* We must reset the transmitter */ > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > > + reg_cache->spcr2 &= (~XRST)); > > > udelay(10); > > > - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, > > > + reg_cache->spcr2 |= XRST); > > > udelay(10); > > > dev_err(mcbsp->dev, "McBSP%d transmitter not " > > > "ready\n", mcbsp->id); > > > @@ -670,9 +679,11 @@ int omap_mcbsp_spi_master_recv_word_poll > > > spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); > > > if (attempts++ > 1000) { > > > /* We must reset the receiver */ > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > > + reg_cache->spcr1 &= (~RRST)); > > > udelay(10); > > > - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, > > > + reg_cache->spcr1 |= RRST); > > > udelay(10); > > > dev_err(mcbsp->dev, "McBSP%d receiver not " > > > "ready\n", mcbsp->id); > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-11-23 23:53 ` Tony Lindgren @ 2009-11-24 7:59 ` Jarkko Nikula 2009-11-24 11:31 ` [RFC][PATCH v2] " Janusz Krzysztofik 0 siblings, 1 reply; 15+ messages in thread From: Jarkko Nikula @ 2009-11-24 7:59 UTC (permalink / raw) To: Tony Lindgren; +Cc: Janusz Krzysztofik, linux-omap@vger.kernel.org On Mon, 23 Nov 2009 15:53:55 -0800 Tony Lindgren <tony@atomide.com> wrote: > > I have just found this patch, initially submitted on 2009-08-12, archived in > > linux-omap patchwork with State: Awaiting Upstream. What does it mean? Am I > > supposed to do something about it? After that many weeks of no negative > > comments nor requests for changes, and taking into account what Jarkko said > > lately in a different thread[*] about the McBSP register cache concept, can I > > assume that it has been generally accepted? Is it worth of refreshing against > > current linux-omap? > > I'll just mark patches "Awaiting upstream" if I feel like somebody else > should ack or merge it. If there are obvious changes needed, I'll mark > it "Changes requested". > > So for the patch, please refresh. Then let's apply it assuming Jarkko > acks it. > Yep, I was aware of this Janusz's patch and was thinking that it could help also for developing the McBSP context save/restore features. > > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = config->spcr2); > > > > + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 = config->spcr1); ... > > > > - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7; > > > > - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7; > > > > + mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7; > > > > + mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7; IMO these would be cleaner if you embed cache handling into read and write functions only. Probably reading could have explicit _read and read_cache functions when code is dealing with the status bits and when just updating some cached bits. -- Jarkko ^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC][PATCH v2] OMAP: McBSP: Use register cache 2009-11-24 7:59 ` Jarkko Nikula @ 2009-11-24 11:31 ` Janusz Krzysztofik 2009-11-26 8:00 ` Jarkko Nikula 0 siblings, 1 reply; 15+ messages in thread From: Janusz Krzysztofik @ 2009-11-24 11:31 UTC (permalink / raw) To: Jarkko Nikula; +Cc: Tony Lindgren, linux-omap@vger.kernel.org Change the way McBSP registers are updated: use cached values instead of relying upon those read back from the device. With this patch, I have finally managed to get rid of all random playback/recording hangups on my OMAP1510 based Amstrad Delta hardware. Before that, values read back from McBSP registers to be used for updating them happened to be errornous. >From the hardware side, the issue appeared to be caused by a relatively high power requirements of an external USB adapter connected to the board's printer dedicated USB port. I think there is one important point that makes this patch worth of applying, apart from my hardware quality. With the current code, if it ever happens to any machine, no matter if OMAP1510 or newer, to read incorrect value from a McBSP register, this wrong value will get written back without any checking. That can lead to hardware damage if, for example, an input pin is turned into output as a result. Created against linux-omap for-next, commit 2963c21fab52bfa8227da7f22864db393ebbc858 Tested on Amstrad Delta Compile tested with omap_generic_2420_defconfig Signed-off-by: Janusz Krzysztofik --- Tuesday 24 November 2009 08:59:23 Jarkko Nikula wrote: > > > > > > + OMAP_MCBSP_WRITE(io_base, SPCR2, reg_cache->spcr2 = > > > > > config->spcr2); + OMAP_MCBSP_WRITE(io_base, SPCR1, reg_cache->spcr1 > > > > > = config->spcr1); > > ... > > > > > > - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & > > > > > 0x7; - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> > > > > > 5) & 0x7; + mcbsp->rx_word_length = (reg_cache->rcr1 >> 5) & 0x7; > > > > > + mcbsp->tx_word_length = (reg_cache->xcr1 >> 5) & 0x7; > > IMO these would be cleaner if you embed cache handling into read and > write functions only. Probably reading could have explicit _read and > read_cache functions when code is dealing with the status bits and when > just updating some cached bits. Hi Jarkko, Could you please have a look at this modified version? It seems to address your comment above more or less closely. Thanks, Janusz diff -upr git.orig/arch/arm/plat-omap/include/plat/mcbsp.h git/arch/arm/plat-omap/include/plat/mcbsp.h --- git.orig/arch/arm/plat-omap/include/plat/mcbsp.h 2009-11-21 00:38:46.000000000 +0100 +++ git/arch/arm/plat-omap/include/plat/mcbsp.h 2009-11-24 02:38:32.000000000 +0100 @@ -415,6 +415,11 @@ struct omap_mcbsp { u16 max_tx_thres; u16 max_rx_thres; #endif +#ifdef CONFIG_ARCH_OMAP1 + u16 reg_cache[OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1]; +#else + u32 reg_cache[OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1]; +#endif }; extern struct omap_mcbsp **mcbsp_ptr; extern int omap_mcbsp_count; Only in git/arch/arm/plat-omap/include/plat: mcbsp.h.orig diff -upr git.orig/arch/arm/plat-omap/mcbsp.c git/arch/arm/plat-omap/mcbsp.c --- git.orig/arch/arm/plat-omap/mcbsp.c 2009-11-21 00:38:46.000000000 +0100 +++ git/arch/arm/plat-omap/mcbsp.c 2009-11-24 12:22:42.000000000 +0100 @@ -46,10 +46,21 @@ int omap_mcbsp_read(void __iomem *io_bas return __raw_readl(io_base + reg); } -#define OMAP_MCBSP_READ(base, reg) \ - omap_mcbsp_read(base, OMAP_MCBSP_REG_##reg) -#define OMAP_MCBSP_WRITE(base, reg, val) \ - omap_mcbsp_write(base, OMAP_MCBSP_REG_##reg, val) +#define OMAP_MCBSP_READ(mcbsp, reg) \ + omap_mcbsp_read(mcbsp->io_base, OMAP_MCBSP_REG_##reg) +#define OMAP_MCBSP_WRITE(mcbsp, reg, val) \ + omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, val) + +#define OMAP_MCBSP_CREAD(mcbsp, reg) \ + (mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1]) +#define OMAP_MCBSP_CWRITE(mcbsp, reg, val) \ + omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, \ + mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \ + = val) + +#define OMAP_MCBSP_BREAD(mcbsp, reg) \ + (mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \ + = OMAP_MCBSP_READ(mcbsp->io_base, reg)) #define omap_mcbsp_check_valid_id(id) (id < omap_mcbsp_count) #define id_to_mcbsp_ptr(id) mcbsp_ptr[id]; @@ -60,31 +71,31 @@ static void omap_mcbsp_dump_reg(u8 id) dev_dbg(mcbsp->dev, "**** McBSP%d regs ****\n", mcbsp->id); dev_dbg(mcbsp->dev, "DRR2: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, DRR2)); + OMAP_MCBSP_READ(mcbsp, DRR2)); dev_dbg(mcbsp->dev, "DRR1: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, DRR1)); + OMAP_MCBSP_READ(mcbsp, DRR1)); dev_dbg(mcbsp->dev, "DXR2: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, DXR2)); + OMAP_MCBSP_READ(mcbsp, DXR2)); dev_dbg(mcbsp->dev, "DXR1: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, DXR1)); + OMAP_MCBSP_READ(mcbsp, DXR1)); dev_dbg(mcbsp->dev, "SPCR2: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, SPCR2)); + OMAP_MCBSP_READ(mcbsp, SPCR2)); dev_dbg(mcbsp->dev, "SPCR1: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, SPCR1)); + OMAP_MCBSP_READ(mcbsp, SPCR1)); dev_dbg(mcbsp->dev, "RCR2: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, RCR2)); + OMAP_MCBSP_READ(mcbsp, RCR2)); dev_dbg(mcbsp->dev, "RCR1: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, RCR1)); + OMAP_MCBSP_READ(mcbsp, RCR1)); dev_dbg(mcbsp->dev, "XCR2: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, XCR2)); + OMAP_MCBSP_READ(mcbsp, XCR2)); dev_dbg(mcbsp->dev, "XCR1: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, XCR1)); + OMAP_MCBSP_READ(mcbsp, XCR1)); dev_dbg(mcbsp->dev, "SRGR2: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, SRGR2)); + OMAP_MCBSP_READ(mcbsp, SRGR2)); dev_dbg(mcbsp->dev, "SRGR1: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, SRGR1)); + OMAP_MCBSP_READ(mcbsp, SRGR1)); dev_dbg(mcbsp->dev, "PCR0: 0x%04x\n", - OMAP_MCBSP_READ(mcbsp->io_base, PCR0)); + OMAP_MCBSP_READ(mcbsp, PCR0)); dev_dbg(mcbsp->dev, "***********************\n"); } @@ -93,15 +104,15 @@ static irqreturn_t omap_mcbsp_tx_irq_han struct omap_mcbsp *mcbsp_tx = dev_id; u16 irqst_spcr2; - irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); + irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx, SPCR2); dev_dbg(mcbsp_tx->dev, "TX IRQ callback : 0x%x\n", irqst_spcr2); if (irqst_spcr2 & XSYNC_ERR) { dev_err(mcbsp_tx->dev, "TX Frame Sync Error! : 0x%x\n", irqst_spcr2); /* Writing zero to XSYNC_ERR clears the IRQ */ - OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, - irqst_spcr2 & ~(XSYNC_ERR)); + OMAP_MCBSP_CWRITE(mcbsp_tx, SPCR2, + OMAP_MCBSP_CREAD(mcbsp_tx, SPCR2) & ~(XSYNC_ERR)); } else { complete(&mcbsp_tx->tx_irq_completion); } @@ -114,15 +125,15 @@ static irqreturn_t omap_mcbsp_rx_irq_han struct omap_mcbsp *mcbsp_rx = dev_id; u16 irqst_spcr1; - irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx->io_base, SPCR1); + irqst_spcr1 = OMAP_MCBSP_READ(mcbsp_rx, SPCR1); dev_dbg(mcbsp_rx->dev, "RX IRQ callback : 0x%x\n", irqst_spcr1); if (irqst_spcr1 & RSYNC_ERR) { dev_err(mcbsp_rx->dev, "RX Frame Sync Error! : 0x%x\n", irqst_spcr1); /* Writing zero to RSYNC_ERR clears the IRQ */ - OMAP_MCBSP_WRITE(mcbsp_rx->io_base, SPCR1, - irqst_spcr1 & ~(RSYNC_ERR)); + OMAP_MCBSP_CWRITE(mcbsp_rx, SPCR1, + OMAP_MCBSP_CREAD(mcbsp_rx, SPCR1) & ~(RSYNC_ERR)); } else { complete(&mcbsp_rx->tx_irq_completion); } @@ -135,7 +146,7 @@ static void omap_mcbsp_tx_dma_callback(i struct omap_mcbsp *mcbsp_dma_tx = data; dev_dbg(mcbsp_dma_tx->dev, "TX DMA callback : 0x%x\n", - OMAP_MCBSP_READ(mcbsp_dma_tx->io_base, SPCR2)); + OMAP_MCBSP_READ(mcbsp_dma_tx, SPCR2)); /* We can free the channels */ omap_free_dma(mcbsp_dma_tx->dma_tx_lch); @@ -149,7 +160,7 @@ static void omap_mcbsp_rx_dma_callback(i struct omap_mcbsp *mcbsp_dma_rx = data; dev_dbg(mcbsp_dma_rx->dev, "RX DMA callback : 0x%x\n", - OMAP_MCBSP_READ(mcbsp_dma_rx->io_base, SPCR2)); + OMAP_MCBSP_READ(mcbsp_dma_rx, SPCR2)); /* We can free the channels */ omap_free_dma(mcbsp_dma_rx->dma_rx_lch); @@ -167,7 +178,6 @@ static void omap_mcbsp_rx_dma_callback(i void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; if (!omap_mcbsp_check_valid_id(id)) { printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1); @@ -175,25 +185,24 @@ void omap_mcbsp_config(unsigned int id, } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; dev_dbg(mcbsp->dev, "Configuring McBSP%d phys_base: 0x%08lx\n", mcbsp->id, mcbsp->phys_base); /* We write the given config */ - OMAP_MCBSP_WRITE(io_base, SPCR2, config->spcr2); - OMAP_MCBSP_WRITE(io_base, SPCR1, config->spcr1); - OMAP_MCBSP_WRITE(io_base, RCR2, config->rcr2); - OMAP_MCBSP_WRITE(io_base, RCR1, config->rcr1); - OMAP_MCBSP_WRITE(io_base, XCR2, config->xcr2); - OMAP_MCBSP_WRITE(io_base, XCR1, config->xcr1); - OMAP_MCBSP_WRITE(io_base, SRGR2, config->srgr2); - OMAP_MCBSP_WRITE(io_base, SRGR1, config->srgr1); - OMAP_MCBSP_WRITE(io_base, MCR2, config->mcr2); - OMAP_MCBSP_WRITE(io_base, MCR1, config->mcr1); - OMAP_MCBSP_WRITE(io_base, PCR0, config->pcr0); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, config->spcr2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, config->spcr1); + OMAP_MCBSP_CWRITE(mcbsp, RCR2, config->rcr2); + OMAP_MCBSP_CWRITE(mcbsp, RCR1, config->rcr1); + OMAP_MCBSP_CWRITE(mcbsp, XCR2, config->xcr2); + OMAP_MCBSP_CWRITE(mcbsp, XCR1, config->xcr1); + OMAP_MCBSP_CWRITE(mcbsp, SRGR2, config->srgr2); + OMAP_MCBSP_CWRITE(mcbsp, SRGR1, config->srgr1); + OMAP_MCBSP_CWRITE(mcbsp, MCR2, config->mcr2); + OMAP_MCBSP_CWRITE(mcbsp, MCR1, config->mcr1); + OMAP_MCBSP_CWRITE(mcbsp, PCR0, config->pcr0); if (cpu_is_omap2430() || cpu_is_omap34xx() || cpu_is_omap44xx()) { - OMAP_MCBSP_WRITE(io_base, XCCR, config->xccr); - OMAP_MCBSP_WRITE(io_base, RCCR, config->rccr); + OMAP_MCBSP_CWRITE(mcbsp, XCCR, config->xccr); + OMAP_MCBSP_CWRITE(mcbsp, RCCR, config->rccr); } } EXPORT_SYMBOL(omap_mcbsp_config); @@ -207,7 +216,6 @@ EXPORT_SYMBOL(omap_mcbsp_config); void omap_mcbsp_set_tx_threshold(unsigned int id, u16 threshold) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; if (!cpu_is_omap34xx()) return; @@ -217,9 +225,8 @@ void omap_mcbsp_set_tx_threshold(unsigne return; } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; - OMAP_MCBSP_WRITE(io_base, THRSH2, threshold); + OMAP_MCBSP_CWRITE(mcbsp, THRSH2, threshold); } EXPORT_SYMBOL(omap_mcbsp_set_tx_threshold); @@ -231,7 +238,6 @@ EXPORT_SYMBOL(omap_mcbsp_set_tx_threshol void omap_mcbsp_set_rx_threshold(unsigned int id, u16 threshold) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; if (!cpu_is_omap34xx()) return; @@ -241,9 +247,8 @@ void omap_mcbsp_set_rx_threshold(unsigne return; } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; - OMAP_MCBSP_WRITE(io_base, THRSH1, threshold); + OMAP_MCBSP_CWRITE(mcbsp, THRSH1, threshold); } EXPORT_SYMBOL(omap_mcbsp_set_rx_threshold); @@ -313,19 +318,18 @@ static inline void omap34xx_mcbsp_reques if (cpu_is_omap34xx()) { u16 syscon; - syscon = OMAP_MCBSP_READ(mcbsp->io_base, SYSCON); + syscon = OMAP_MCBSP_READ(mcbsp, SYSCON); syscon &= ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); if (mcbsp->dma_op_mode == MCBSP_DMA_MODE_THRESHOLD) { syscon |= (ENAWAKEUP | SIDLEMODE(0x02) | CLOCKACTIVITY(0x02)); - OMAP_MCBSP_WRITE(mcbsp->io_base, WAKEUPEN, - XRDYEN | RRDYEN); + OMAP_MCBSP_CWRITE(mcbsp, WAKEUPEN, XRDYEN | RRDYEN); } else { syscon |= SIDLEMODE(0x01); } - OMAP_MCBSP_WRITE(mcbsp->io_base, SYSCON, syscon); + OMAP_MCBSP_CWRITE(mcbsp, SYSCON, syscon); } } @@ -337,7 +341,7 @@ static inline void omap34xx_mcbsp_free(s if (cpu_is_omap34xx()) { u16 syscon; - syscon = OMAP_MCBSP_READ(mcbsp->io_base, SYSCON); + syscon = OMAP_MCBSP_READ(mcbsp, SYSCON); syscon &= ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); /* * HW bug workaround - If no_idle mode is taken, we need to @@ -345,12 +349,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); + OMAP_MCBSP_CWRITE(mcbsp, SYSCON, syscon); syscon &= ~(SIDLEMODE(0x03)); - OMAP_MCBSP_WRITE(mcbsp->io_base, SYSCON, syscon); + OMAP_MCBSP_CWRITE(mcbsp, SYSCON, syscon); - OMAP_MCBSP_WRITE(mcbsp->io_base, WAKEUPEN, 0); + OMAP_MCBSP_CWRITE(mcbsp, WAKEUPEN, 0); } } #else @@ -424,8 +428,8 @@ int omap_mcbsp_request(unsigned int id) * Make sure that transmitter, receiver and sample-rate generator are * not running before activating IRQs. */ - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR1, 0); - OMAP_MCBSP_WRITE(mcbsp->io_base, SPCR2, 0); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, 0); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, 0); if (mcbsp->io_type == OMAP_MCBSP_IRQ_IO) { /* We need to get IRQs here */ @@ -501,7 +505,6 @@ EXPORT_SYMBOL(omap_mcbsp_free); void omap_mcbsp_start(unsigned int id, int tx, int rx) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; int idle; u16 w; @@ -510,28 +513,27 @@ void omap_mcbsp_start(unsigned int id, i return; } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; - mcbsp->rx_word_length = (OMAP_MCBSP_READ(io_base, RCR1) >> 5) & 0x7; - mcbsp->tx_word_length = (OMAP_MCBSP_READ(io_base, XCR1) >> 5) & 0x7; + mcbsp->rx_word_length = (OMAP_MCBSP_CREAD(mcbsp, RCR1) >> 5) & 0x7; + mcbsp->tx_word_length = (OMAP_MCBSP_CREAD(mcbsp, XCR1) >> 5) & 0x7; - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); + idle = !((OMAP_MCBSP_CREAD(mcbsp, SPCR2) | + OMAP_MCBSP_CREAD(mcbsp, SPCR1)) & 1); if (idle) { /* Start the sample generator */ - w = OMAP_MCBSP_READ(io_base, SPCR2); - OMAP_MCBSP_WRITE(io_base, SPCR2, w | (1 << 6)); + w = OMAP_MCBSP_CREAD(mcbsp, SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, w | (1 << 6)); } /* Enable transmitter and receiver */ tx &= 1; - w = OMAP_MCBSP_READ(io_base, SPCR2); - OMAP_MCBSP_WRITE(io_base, SPCR2, w | tx); + w = OMAP_MCBSP_CREAD(mcbsp, SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, w | tx); rx &= 1; - w = OMAP_MCBSP_READ(io_base, SPCR1); - OMAP_MCBSP_WRITE(io_base, SPCR1, w | rx); + w = OMAP_MCBSP_CREAD(mcbsp, SPCR1); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, w | rx); /* * Worst case: CLKSRG*2 = 8000khz: (1/8000) * 2 * 2 usec @@ -543,18 +545,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 = OMAP_MCBSP_CREAD(mcbsp, SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, w | (1 << 7)); } if (cpu_is_omap2430() || cpu_is_omap34xx()) { /* Release the transmitter and receiver */ - w = OMAP_MCBSP_READ(io_base, XCCR); + w = OMAP_MCBSP_CREAD(mcbsp, XCCR); w &= ~(tx ? XDISABLE : 0); - OMAP_MCBSP_WRITE(io_base, XCCR, w); - w = OMAP_MCBSP_READ(io_base, RCCR); + OMAP_MCBSP_CWRITE(mcbsp, XCCR, w); + w = OMAP_MCBSP_CREAD(mcbsp, RCCR); w &= ~(rx ? RDISABLE : 0); - OMAP_MCBSP_WRITE(io_base, RCCR, w); + OMAP_MCBSP_CWRITE(mcbsp, RCCR, w); } /* Dump McBSP Regs */ @@ -565,7 +567,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 +576,34 @@ 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()) { - w = OMAP_MCBSP_READ(io_base, XCCR); + w = OMAP_MCBSP_CREAD(mcbsp, XCCR); w |= (tx ? XDISABLE : 0); - OMAP_MCBSP_WRITE(io_base, XCCR, w); + OMAP_MCBSP_CWRITE(mcbsp, XCCR, w); } - w = OMAP_MCBSP_READ(io_base, SPCR2); - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~tx); + w = OMAP_MCBSP_CREAD(mcbsp, SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, w & ~tx); /* Reset receiver */ rx &= 1; if (cpu_is_omap2430() || cpu_is_omap34xx()) { - w = OMAP_MCBSP_READ(io_base, RCCR); + w = OMAP_MCBSP_CREAD(mcbsp, RCCR); w |= (rx ? RDISABLE : 0); - OMAP_MCBSP_WRITE(io_base, RCCR, w); + OMAP_MCBSP_CWRITE(mcbsp, RCCR, w); } - w = OMAP_MCBSP_READ(io_base, SPCR1); - OMAP_MCBSP_WRITE(io_base, SPCR1, w & ~rx); + w = OMAP_MCBSP_CREAD(mcbsp, SPCR1); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, w & ~rx); - idle = !((OMAP_MCBSP_READ(io_base, SPCR2) | - OMAP_MCBSP_READ(io_base, SPCR1)) & 1); + idle = !((OMAP_MCBSP_CREAD(mcbsp, SPCR2) | + OMAP_MCBSP_CREAD(mcbsp, SPCR1)) & 1); if (idle) { /* Reset the sample rate generator */ - w = OMAP_MCBSP_READ(io_base, SPCR2); - OMAP_MCBSP_WRITE(io_base, SPCR2, w & ~(1 << 6)); + w = OMAP_MCBSP_CREAD(mcbsp, SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, w & ~(1 << 6)); } } EXPORT_SYMBOL(omap_mcbsp_stop); @@ -612,7 +612,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 +619,27 @@ int omap_mcbsp_pollwrite(unsigned int id } mcbsp = id_to_mcbsp_ptr(id); - base = mcbsp->io_base; - writew(buf, base + OMAP_MCBSP_REG_DXR1); + OMAP_MCBSP_WRITE(mcbsp, DXR1, buf); /* if frame sync error - clear the error */ - if (readw(base + OMAP_MCBSP_REG_SPCR2) & XSYNC_ERR) { + if (OMAP_MCBSP_READ(mcbsp, SPCR2) & XSYNC_ERR) { /* clear error */ - writew(readw(base + OMAP_MCBSP_REG_SPCR2) & (~XSYNC_ERR), - base + OMAP_MCBSP_REG_SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, 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(mcbsp, SPCR2) & XRDY)) { if (attemps++ > 1000) { - writew(readw(base + OMAP_MCBSP_REG_SPCR2) & - (~XRST), - base + OMAP_MCBSP_REG_SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, SPCR2) & + (~XRST)); udelay(10); - writew(readw(base + OMAP_MCBSP_REG_SPCR2) | - (XRST), - base + OMAP_MCBSP_REG_SPCR2); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, SPCR2) | + (XRST)); udelay(10); dev_err(mcbsp->dev, "Could not write to" " McBSP%d Register\n", mcbsp->id); @@ -657,7 +655,6 @@ EXPORT_SYMBOL(omap_mcbsp_pollwrite); int omap_mcbsp_pollread(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); @@ -665,26 +662,25 @@ int omap_mcbsp_pollread(unsigned int id, } mcbsp = id_to_mcbsp_ptr(id); - base = mcbsp->io_base; /* if frame sync error - clear the error */ - if (readw(base + OMAP_MCBSP_REG_SPCR1) & RSYNC_ERR) { + if (OMAP_MCBSP_READ(mcbsp, SPCR1) & RSYNC_ERR) { /* clear error */ - writew(readw(base + OMAP_MCBSP_REG_SPCR1) & (~RSYNC_ERR), - base + OMAP_MCBSP_REG_SPCR1); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) & (~RSYNC_ERR)); /* resend */ return -1; } else { /* wait for recieve confirmation */ int attemps = 0; - while (!(readw(base + OMAP_MCBSP_REG_SPCR1) & RRDY)) { + while (!(OMAP_MCBSP_READ(mcbsp, SPCR1) & RRDY)) { if (attemps++ > 1000) { - writew(readw(base + OMAP_MCBSP_REG_SPCR1) & - (~RRST), - base + OMAP_MCBSP_REG_SPCR1); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) & + (~RRST)); udelay(10); - writew(readw(base + OMAP_MCBSP_REG_SPCR1) | - (RRST), - base + OMAP_MCBSP_REG_SPCR1); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) | + (RRST)); udelay(10); dev_err(mcbsp->dev, "Could not read from" " McBSP%d Register\n", mcbsp->id); @@ -692,7 +688,7 @@ int omap_mcbsp_pollread(unsigned int id, } } } - *buf = readw(base + OMAP_MCBSP_REG_DRR1); + *buf = OMAP_MCBSP_READ(mcbsp, DRR1); return 0; } @@ -704,7 +700,6 @@ EXPORT_SYMBOL(omap_mcbsp_pollread); void omap_mcbsp_xmit_word(unsigned int id, u32 word) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; omap_mcbsp_word_length word_length; if (!omap_mcbsp_check_valid_id(id)) { @@ -713,21 +708,19 @@ void omap_mcbsp_xmit_word(unsigned int i } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; word_length = mcbsp->tx_word_length; wait_for_completion(&mcbsp->tx_irq_completion); if (word_length > OMAP_MCBSP_WORD_16) - OMAP_MCBSP_WRITE(io_base, DXR2, word >> 16); - OMAP_MCBSP_WRITE(io_base, DXR1, word & 0xffff); + OMAP_MCBSP_WRITE(mcbsp, DXR2, word >> 16); + OMAP_MCBSP_WRITE(mcbsp, DXR1, word & 0xffff); } EXPORT_SYMBOL(omap_mcbsp_xmit_word); u32 omap_mcbsp_recv_word(unsigned int id) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; u16 word_lsb, word_msb = 0; omap_mcbsp_word_length word_length; @@ -738,13 +731,12 @@ u32 omap_mcbsp_recv_word(unsigned int id mcbsp = id_to_mcbsp_ptr(id); word_length = mcbsp->rx_word_length; - io_base = mcbsp->io_base; wait_for_completion(&mcbsp->rx_irq_completion); if (word_length > OMAP_MCBSP_WORD_16) - word_msb = OMAP_MCBSP_READ(io_base, DRR2); - word_lsb = OMAP_MCBSP_READ(io_base, DRR1); + word_msb = OMAP_MCBSP_READ(mcbsp, DRR2); + word_lsb = OMAP_MCBSP_READ(mcbsp, DRR1); return (word_lsb | (word_msb << 16)); } @@ -753,7 +745,6 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word); int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word) { struct omap_mcbsp *mcbsp; - void __iomem *io_base; omap_mcbsp_word_length tx_word_length; omap_mcbsp_word_length rx_word_length; u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0; @@ -763,7 +754,6 @@ int omap_mcbsp_spi_master_xmit_word_poll return -ENODEV; } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; tx_word_length = mcbsp->tx_word_length; rx_word_length = mcbsp->rx_word_length; @@ -771,14 +761,16 @@ int omap_mcbsp_spi_master_xmit_word_poll return -EINVAL; /* First we wait for the transmitter to be ready */ - spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); + spcr2 = OMAP_MCBSP_READ(mcbsp, SPCR2); while (!(spcr2 & XRDY)) { - spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); + spcr2 = OMAP_MCBSP_READ(mcbsp, SPCR2); if (attempts++ > 1000) { /* We must reset the transmitter */ - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, SPCR2) & (~XRST)); udelay(10); - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, SPCR2) | XRST); udelay(10); dev_err(mcbsp->dev, "McBSP%d transmitter not " "ready\n", mcbsp->id); @@ -788,18 +780,20 @@ int omap_mcbsp_spi_master_xmit_word_poll /* Now we can push the data */ if (tx_word_length > OMAP_MCBSP_WORD_16) - OMAP_MCBSP_WRITE(io_base, DXR2, word >> 16); - OMAP_MCBSP_WRITE(io_base, DXR1, word & 0xffff); + OMAP_MCBSP_WRITE(mcbsp, DXR2, word >> 16); + OMAP_MCBSP_WRITE(mcbsp, DXR1, word & 0xffff); /* We wait for the receiver to be ready */ - spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); + spcr1 = OMAP_MCBSP_READ(mcbsp, SPCR1); while (!(spcr1 & RRDY)) { - spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); + spcr1 = OMAP_MCBSP_READ(mcbsp, SPCR1); if (attempts++ > 1000) { /* We must reset the receiver */ - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) & (~RRST)); udelay(10); - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) | RRST); udelay(10); dev_err(mcbsp->dev, "McBSP%d receiver not " "ready\n", mcbsp->id); @@ -809,8 +803,8 @@ int omap_mcbsp_spi_master_xmit_word_poll /* Receiver is ready, let's read the dummy data */ if (rx_word_length > OMAP_MCBSP_WORD_16) - word_msb = OMAP_MCBSP_READ(io_base, DRR2); - word_lsb = OMAP_MCBSP_READ(io_base, DRR1); + word_msb = OMAP_MCBSP_READ(mcbsp, DRR2); + word_lsb = OMAP_MCBSP_READ(mcbsp, DRR1); return 0; } @@ -820,7 +814,6 @@ int omap_mcbsp_spi_master_recv_word_poll { struct omap_mcbsp *mcbsp; u32 clock_word = 0; - void __iomem *io_base; omap_mcbsp_word_length tx_word_length; omap_mcbsp_word_length rx_word_length; u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0; @@ -831,7 +824,6 @@ int omap_mcbsp_spi_master_recv_word_poll } mcbsp = id_to_mcbsp_ptr(id); - io_base = mcbsp->io_base; tx_word_length = mcbsp->tx_word_length; rx_word_length = mcbsp->rx_word_length; @@ -840,14 +832,16 @@ int omap_mcbsp_spi_master_recv_word_poll return -EINVAL; /* First we wait for the transmitter to be ready */ - spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); + spcr2 = OMAP_MCBSP_READ(mcbsp, SPCR2); while (!(spcr2 & XRDY)) { - spcr2 = OMAP_MCBSP_READ(io_base, SPCR2); + spcr2 = OMAP_MCBSP_READ(mcbsp, SPCR2); if (attempts++ > 1000) { /* We must reset the transmitter */ - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 & (~XRST)); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, SPCR2) & (~XRST)); udelay(10); - OMAP_MCBSP_WRITE(io_base, SPCR2, spcr2 | XRST); + OMAP_MCBSP_CWRITE(mcbsp, SPCR2, + OMAP_MCBSP_CREAD(mcbsp, SPCR2) | XRST); udelay(10); dev_err(mcbsp->dev, "McBSP%d transmitter not " "ready\n", mcbsp->id); @@ -857,18 +851,20 @@ int omap_mcbsp_spi_master_recv_word_poll /* We first need to enable the bus clock */ if (tx_word_length > OMAP_MCBSP_WORD_16) - OMAP_MCBSP_WRITE(io_base, DXR2, clock_word >> 16); - OMAP_MCBSP_WRITE(io_base, DXR1, clock_word & 0xffff); + OMAP_MCBSP_WRITE(mcbsp, DXR2, clock_word >> 16); + OMAP_MCBSP_WRITE(mcbsp, DXR1, clock_word & 0xffff); /* We wait for the receiver to be ready */ - spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); + spcr1 = OMAP_MCBSP_READ(mcbsp, SPCR1); while (!(spcr1 & RRDY)) { - spcr1 = OMAP_MCBSP_READ(io_base, SPCR1); + spcr1 = OMAP_MCBSP_READ(mcbsp, SPCR1); if (attempts++ > 1000) { /* We must reset the receiver */ - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 & (~RRST)); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) & (~RRST)); udelay(10); - OMAP_MCBSP_WRITE(io_base, SPCR1, spcr1 | RRST); + OMAP_MCBSP_CWRITE(mcbsp, SPCR1, + OMAP_MCBSP_CREAD(mcbsp, SPCR1) | RRST); udelay(10); dev_err(mcbsp->dev, "McBSP%d receiver not " "ready\n", mcbsp->id); @@ -878,8 +874,8 @@ int omap_mcbsp_spi_master_recv_word_poll /* Receiver is ready, there is something for us */ if (rx_word_length > OMAP_MCBSP_WORD_16) - word_msb = OMAP_MCBSP_READ(io_base, DRR2); - word_lsb = OMAP_MCBSP_READ(io_base, DRR1); + word_msb = OMAP_MCBSP_READ(mcbsp, DRR2); + word_lsb = OMAP_MCBSP_READ(mcbsp, DRR1); word[0] = (word_lsb | (word_msb << 16)); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH v2] OMAP: McBSP: Use register cache 2009-11-24 11:31 ` [RFC][PATCH v2] " Janusz Krzysztofik @ 2009-11-26 8:00 ` Jarkko Nikula 2009-11-26 13:25 ` Janusz Krzysztofik 0 siblings, 1 reply; 15+ messages in thread From: Jarkko Nikula @ 2009-11-26 8:00 UTC (permalink / raw) To: Janusz Krzysztofik; +Cc: Tony Lindgren, linux-omap@vger.kernel.org Hi On Tue, 24 Nov 2009 12:31:16 +0100 Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote: > -#define OMAP_MCBSP_READ(base, reg) \ > - omap_mcbsp_read(base, OMAP_MCBSP_REG_##reg) > -#define OMAP_MCBSP_WRITE(base, reg, val) \ > - omap_mcbsp_write(base, OMAP_MCBSP_REG_##reg, val) > +#define OMAP_MCBSP_READ(mcbsp, reg) \ > + omap_mcbsp_read(mcbsp->io_base, OMAP_MCBSP_REG_##reg) > +#define OMAP_MCBSP_WRITE(mcbsp, reg, val) \ > + omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, val) > + > +#define OMAP_MCBSP_CREAD(mcbsp, reg) \ > + (mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1]) > +#define OMAP_MCBSP_CWRITE(mcbsp, reg, val) \ > + omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, \ > + mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \ > + = val) > + Have rather single write writing both the cache and actual register. I.e. keep the OMAP_MCBSP_WRITE since writes should always go to hw and makes the patch smaller. I also found the OMAP_MCBSP_CREAD a little unclear so I suggest to name is as OMAP_MCBSP_READ_CACHE. > +#define OMAP_MCBSP_BREAD(mcbsp, reg) \ > + (mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \ > + = OMAP_MCBSP_READ(mcbsp->io_base, reg)) > Why is this? There is nothing eating this :-) > dev_dbg(mcbsp->dev, "DRR2: 0x%04x\n", > - OMAP_MCBSP_READ(mcbsp->io_base, DRR2)); > + OMAP_MCBSP_READ(mcbsp, DRR2)); These changes are worth to send as a separate cleanup patch. Will make the actual cache patch smaller and easier to follow. > @@ -93,15 +104,15 @@ static irqreturn_t omap_mcbsp_tx_irq_han > struct omap_mcbsp *mcbsp_tx = dev_id; > u16 irqst_spcr2; > > - irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); > + irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx, SPCR2); > dev_dbg(mcbsp_tx->dev, "TX IRQ callback : 0x%x\n", irqst_spcr2); > > if (irqst_spcr2 & XSYNC_ERR) { > dev_err(mcbsp_tx->dev, "TX Frame Sync Error! : 0x%x\n", > irqst_spcr2); > /* Writing zero to XSYNC_ERR clears the IRQ */ > - OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, > - irqst_spcr2 & ~(XSYNC_ERR)); > + OMAP_MCBSP_CWRITE(mcbsp_tx, SPCR2, > + OMAP_MCBSP_CREAD(mcbsp_tx, SPCR2) & ~(XSYNC_ERR)); I was thinking that should these read+read_cache changes be a separate patch for fixing the 1510 issues since no other OMAPs are known to corrupt registers and plain hw read is enough for them. > @@ -612,7 +612,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 +619,27 @@ int omap_mcbsp_pollwrite(unsigned int id > } > > mcbsp = id_to_mcbsp_ptr(id); > - base = mcbsp->io_base; > > - writew(buf, base + OMAP_MCBSP_REG_DXR1); > + OMAP_MCBSP_WRITE(mcbsp, DXR1, buf); > /* if frame sync error - clear the error */ > - if (readw(base + OMAP_MCBSP_REG_SPCR2) & XSYNC_ERR) { > + if (OMAP_MCBSP_READ(mcbsp, SPCR2) & XSYNC_ERR) { These looks also that these are better to cover with its own separate cleanup patch. --- I'm not completely sure are there any code path expecting to read reset default values from the cache or are there always explicit write or read before it? I was thinking would it be necessary to initialize the cache by issuing dummy reads for all registers. IRCC the OMAP2420 has fewer registers than 2430 or OMAP3 so that should be took into account if there is a need to do so. -- Jarkko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH v2] OMAP: McBSP: Use register cache 2009-11-26 8:00 ` Jarkko Nikula @ 2009-11-26 13:25 ` Janusz Krzysztofik 2009-11-28 16:14 ` Jarkko Nikula 0 siblings, 1 reply; 15+ messages in thread From: Janusz Krzysztofik @ 2009-11-26 13:25 UTC (permalink / raw) To: Jarkko Nikula; +Cc: Tony Lindgren, linux-omap@vger.kernel.org Thursday 26 November 2009 09:00:00 Jarkko Nikula napisał(a): > Hi > > On Tue, 24 Nov 2009 12:31:16 +0100 > > Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote: > > -#define OMAP_MCBSP_READ(base, reg) \ > > - omap_mcbsp_read(base, OMAP_MCBSP_REG_##reg) > > -#define OMAP_MCBSP_WRITE(base, reg, val) \ > > - omap_mcbsp_write(base, OMAP_MCBSP_REG_##reg, val) > > +#define OMAP_MCBSP_READ(mcbsp, reg) \ > > + omap_mcbsp_read(mcbsp->io_base, OMAP_MCBSP_REG_##reg) > > +#define OMAP_MCBSP_WRITE(mcbsp, reg, val) \ > > + omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, val) > > + > > +#define OMAP_MCBSP_CREAD(mcbsp, reg) \ > > + (mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1]) > > +#define OMAP_MCBSP_CWRITE(mcbsp, reg, val) \ > > + omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, \ > > + mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \ > > + = val) > > + > > Have rather single write writing both the cache and actual register. > I.e. keep the OMAP_MCBSP_WRITE since writes should always go to hw and > makes the patch smaller. Jarkko, Not that I am against making things simpler, but please reconsider: 1. Is there any point for caching data written to DXR registers? 2. There is at least one register, WAKEUPEN, never updated bit by bit, but always rewritten from scratch with a few different values that can be computed easily from other msbsp structure member values. For these rare cases, is it any worth to bypass the cache while writing them, saving a few instructions maybe? 3. Sometimes a register is written several times in succession with different values, like SYSCON inside omap34xx_mcbsp_free(), for example. Could all writes but the last one bypass the cache for the same reason? Another axample is omap_mcbsp_start() fiddling with SPCR2 several times. However, there is a udelay(500) inbetween that I am not sure if can ever happen to be interleaved with a concurrent suspend or idle or something like that requiring the cache being up-to-date. That said, please confirm if a single write macro with caching still seems sufficient, or a separate one without caching would provide enough benefits to keep it as well. > I also found the OMAP_MCBSP_CREAD a little > unclear so I suggest to name is as OMAP_MCBSP_READ_CACHE. I just tried to minimize line wrapping :/. If we agree on a single caching version of OMAP_MCBSP_WRITE(), what about OMAP_CACHE_READ()? If no better ideas, I'll use what you suggest. > > +#define OMAP_MCBSP_BREAD(mcbsp, reg) \ > > + (mcbsp->reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \ > > + = OMAP_MCBSP_READ(mcbsp->io_base, reg)) > > Why is this? There is nothing eating this :-) It's a part of a possible solution I am considering for SYSCON register handling. See below. > > dev_dbg(mcbsp->dev, "DRR2: 0x%04x\n", > > - OMAP_MCBSP_READ(mcbsp->io_base, DRR2)); > > + OMAP_MCBSP_READ(mcbsp, DRR2)); > > These changes are worth to send as a separate cleanup patch. Will make > the actual cache patch smaller and easier to follow. OK, but let me recap that first. We have two options: 1. All macros accept very same arguments, as long as applicable. This is what I have put in my patch. 2. Every macro is optimized in respect of its argumnets, ie. OMAP_MCBSP_READ(io_base, ...) OMAP_MCBSP_READ_CACHE(reg_cache, ...) OMAP_MCBSP_WRITE(mcbsp, ...) Which one do you think is better? > > @@ -93,15 +104,15 @@ static irqreturn_t omap_mcbsp_tx_irq_han > > struct omap_mcbsp *mcbsp_tx = dev_id; > > u16 irqst_spcr2; > > > > - irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx->io_base, SPCR2); > > + irqst_spcr2 = OMAP_MCBSP_READ(mcbsp_tx, SPCR2); > > dev_dbg(mcbsp_tx->dev, "TX IRQ callback : 0x%x\n", irqst_spcr2); > > > > if (irqst_spcr2 & XSYNC_ERR) { > > dev_err(mcbsp_tx->dev, "TX Frame Sync Error! : 0x%x\n", > > irqst_spcr2); > > /* Writing zero to XSYNC_ERR clears the IRQ */ > > - OMAP_MCBSP_WRITE(mcbsp_tx->io_base, SPCR2, > > - irqst_spcr2 & ~(XSYNC_ERR)); > > + OMAP_MCBSP_CWRITE(mcbsp_tx, SPCR2, > > + OMAP_MCBSP_CREAD(mcbsp_tx, SPCR2) & ~(XSYNC_ERR)); > > I was thinking that should these read+read_cache changes be a separate > patch for fixing the 1510 issues since no other OMAPs are known to > corrupt registers and plain hw read is enough for them. I see. Do you want the change limited to cpu_is_omap1510() case only? > > @@ -612,7 +612,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 +619,27 @@ int omap_mcbsp_pollwrite(unsigned int id > > } > > > > mcbsp = id_to_mcbsp_ptr(id); > > - base = mcbsp->io_base; > > > > - writew(buf, base + OMAP_MCBSP_REG_DXR1); > > + OMAP_MCBSP_WRITE(mcbsp, DXR1, buf); > > /* if frame sync error - clear the error */ > > - if (readw(base + OMAP_MCBSP_REG_SPCR2) & XSYNC_ERR) { > > + if (OMAP_MCBSP_READ(mcbsp, SPCR2) & XSYNC_ERR) { > > These looks also that these are better to cover with its own separate > cleanup patch. Ok, the series will start with this cleanup therefore. > I'm not completely sure are there any code path expecting to read reset > default values from the cache or are there always explicit write or > read before it? I was thinking would it be necessary to initialize the > cache by issuing dummy reads for all registers. IRCC the OMAP2420 has > fewer registers than 2430 or OMAP3 so that should be took into account > if there is a need to do so. First of all, I decided not touching any registers not maintained by the code so far. I have not enough knowledge to deal with them unless someone decides to shepherd me there :-). The only issue I am not satisfied how I have dealt with so far is SYSCON register case. I have noticed that the register is never initialized like others , but always updated based on values read back from hardware. Initially I didn't find any good idea how to deal with this in respect of caching, so decided to take an intermediate path: read from hardware, then write with caching, even if the cached value is never referred for now. Here is the relevant part of my patch: @@ -313,19 +318,18 @@ static inline void omap34xx_mcbsp_reques if (cpu_is_omap34xx()) { u16 syscon; - syscon = OMAP_MCBSP_READ(mcbsp->io_base, SYSCON); + syscon = OMAP_MCBSP_READ(mcbsp, SYSCON); syscon &= ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); if (mcbsp->dma_op_mode == MCBSP_DMA_MODE_THRESHOLD) { syscon |= (ENAWAKEUP | SIDLEMODE(0x02) | CLOCKACTIVITY(0x02)); - OMAP_MCBSP_WRITE(mcbsp->io_base, WAKEUPEN, - XRDYEN | RRDYEN); + OMAP_MCBSP_CWRITE(mcbsp, WAKEUPEN, XRDYEN | RRDYEN); } else { syscon |= SIDLEMODE(0x01); } - OMAP_MCBSP_WRITE(mcbsp->io_base, SYSCON, syscon); + OMAP_MCBSP_CWRITE(mcbsp, SYSCON, syscon); } } @@ -337,7 +341,7 @@ static inline void omap34xx_mcbsp_free(s if (cpu_is_omap34xx()) { u16 syscon; - syscon = OMAP_MCBSP_READ(mcbsp->io_base, SYSCON); + syscon = OMAP_MCBSP_READ(mcbsp, SYSCON); syscon &= ~(ENAWAKEUP | SIDLEMODE(0x03) | CLOCKACTIVITY(0x03)); /* * HW bug workaround - If no_idle mode is taken, we need to @@ -345,12 +349,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); + OMAP_MCBSP_CWRITE(mcbsp, SYSCON, syscon); syscon &= ~(SIDLEMODE(0x03)); - OMAP_MCBSP_WRITE(mcbsp->io_base, SYSCON, syscon); + OMAP_MCBSP_CWRITE(mcbsp, SYSCON, syscon); - OMAP_MCBSP_WRITE(mcbsp->io_base, WAKEUPEN, 0); + OMAP_MCBSP_CWRITE(mcbsp, WAKEUPEN, 0); } } #else How about caching the SYSCON register initial value from inside omap_mcbsp_config() by copying it's initial value from the hardware with something like that OMAP_MCBSP_BREAD() macro not eaten so far (probably renamed to OMAP_CACHE_INIT/COPY() or OMAP_MCBSP_SAVE/STORE() or something similiar)? Would that be enough for addressing your concern? Thanks, Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH v2] OMAP: McBSP: Use register cache 2009-11-26 13:25 ` Janusz Krzysztofik @ 2009-11-28 16:14 ` Jarkko Nikula 0 siblings, 0 replies; 15+ messages in thread From: Jarkko Nikula @ 2009-11-28 16:14 UTC (permalink / raw) To: Janusz Krzysztofik; +Cc: Tony Lindgren, linux-omap@vger.kernel.org Hi On Thu, 26 Nov 2009 14:25:36 +0100 Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> wrote: > 1. Is there any point for caching data written to DXR registers? > > 2. There is at least one register, WAKEUPEN, never updated bit by bit, but > always rewritten from scratch with a few different values that can be > computed easily from other msbsp structure member values. For these rare > cases, is it any worth to bypass the cache while writing them, saving a few > instructions maybe? > I would say it's better to favor simplicity and uniformity first. Trying to avoid caching for few registers makes the cache code harder to follow, more error prone and may not save the total memory footprint. > 3. Sometimes a register is written several times in succession with different > values, like SYSCON inside omap34xx_mcbsp_free(), for example. Could all > writes but the last one bypass the cache for the same reason? > > Another axample is omap_mcbsp_start() fiddling with SPCR2 several times. > However, there is a udelay(500) inbetween that I am not sure if can ever > happen to be interleaved with a concurrent suspend or idle or something like > that requiring the cache being up-to-date. > I'm not sure about SYSCON but there are registers like SPCR2 where specifications and practical findings define the minimum times between activating the logic so caching must not optimize those away. > That said, please confirm if a single write macro with caching still seems > sufficient, or a separate one without caching would provide enough benefits > to keep it as well. > Single write with caching would be sufficient I would say. > I just tried to minimize line wrapping :/. If we agree on a single caching > version of OMAP_MCBSP_WRITE(), what about OMAP_CACHE_READ()? If no better > ideas, I'll use what you suggest. > Ok, I see. How about these? OMAP is obvious in this case so we can drop it off from macro names. MCBSP_WRITE MCBSP_READ MCBSP_READ_CACHE > 1. All macros accept very same arguments, as long as applicable. > This is what I have put in my patch. > Definitely this. Pass instance of "struct omap_mcbsp" as you did since all other data is found from there. > > I was thinking that should these read+read_cache changes be a separate > > patch for fixing the 1510 issues since no other OMAPs are known to > > corrupt registers and plain hw read is enough for them. > > I see. Do you want the change limited to cpu_is_omap1510() case only? > I think that's not necessary. The separate patch would explain that the change is fixing 1510 issue and the code is fine for other OMAPs as well. > The only issue I am not satisfied how I have dealt with so far is SYSCON > register case. I have noticed that the register is never initialized like > others , but always updated based on values read back from hardware. > Initially I didn't find any good idea how to deal with this in respect of > caching, so decided to take an intermediate path: read from hardware, then > write with caching, even if the cached value is never referred for now. Here > is the relevant part of my patch: > Probably simplest and safest is to do so that one patch will change the macro arguments and rename them, another introducing caching with adding _READ_CACHE and then one for 1510 fix. Then the code behaves the same and more _READ_CACHE changes can be introduced gradually by anothers if needed. -- Jarkko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache @ 2009-09-04 5:39 Eero Nurkkala 2009-09-04 10:34 ` Janusz Krzysztofik 0 siblings, 1 reply; 15+ messages in thread From: Eero Nurkkala @ 2009-09-04 5:39 UTC (permalink / raw) To: linux-omap Hello, Register values should not get corrupted. Could you please point out the registers that may get corrupted? Do they get corrupted during audio play? I haven't seen corrupted register values. - Eero ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-09-04 5:39 [RFC][PATCH] ARM: " Eero Nurkkala @ 2009-09-04 10:34 ` Janusz Krzysztofik 2009-09-04 11:07 ` Eero Nurkkala 0 siblings, 1 reply; 15+ messages in thread From: Janusz Krzysztofik @ 2009-09-04 10:34 UTC (permalink / raw) To: ext-eero.nurkkala; +Cc: linux-omap, e3-hacking Friday 04 September 2009 07:39:36 Eero Nurkkala wrote: > > Register values should not get corrupted. Could you please point out the > registers that may get corrupted? Do they get corrupted during audio > play? I haven't seen corrupted register values. > Hi Eero, Please have a look at some debug log lines below. Those were generated on my Amstrad Delta videophone, using an application (asterisk softpbx with alsa channel) that can restart a receiver of a prevoiusly requested McBSP by calling omap_mcbsp_start() on user request. As you can see, corrupted values can happen to get read from any register that is expected to return predictable results. Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: **** McBSP1 regs **** Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR2: 0x1000 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR1: 0x009d Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR2: 0x3000 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR1: 0x7da1 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR2: 0x02f0 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR1: 0x7031 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR2: 0x0045 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR1: 0x0040 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR2: 0x1045 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR1: 0x0040 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR2: 0x310f Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR1: 0x0000 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: PCR0: 0x0003 Sep 4 08:21:17 amsdelta user.debug kernel: [309910.170000] omap-mcbsp omap-mcbsp.1: mcbsp: *********************** Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: **** McBSP1 regs **** Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR2: 0x0000 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR1: 0x1086 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR2: 0x0000 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR1: 0x7da1 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR2: 0x02f0 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR1: 0x0031 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR2: 0x1045 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR1: 0x1040 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR2: 0x3045 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR1: 0x1040 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR2: 0x000f Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR1: 0x0000 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: PCR0: 0x1003 Sep 4 08:21:19 amsdelta user.debug kernel: [309912.050000] omap-mcbsp omap-mcbsp.1: mcbsp: *********************** Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: **** McBSP1 regs **** Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR2: 0x1000 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DRR1: 0x109e Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR2: 0x1000 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: DXR1: 0x7da1 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR2: 0x02f0 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SPCR1: 0x1031 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR2: 0x1045 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: RCR1: 0x1040 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR2: 0x2045 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: XCR1: 0x1040 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR2: 0x100f Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: SRGR1: 0x1000 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: PCR0: 0x2003 Sep 4 08:21:21 amsdelta user.debug kernel: [309913.950000] omap-mcbsp omap-mcbsp.1: mcbsp: *********************** Those corruptions, even if should never happen, were actually caused by a wireless adapter connected to the machine USB port. Extra piece of hardware, very usefull in some cases, but obviously not taken into account while the machine was being designed. While playback seems unaffected, WiFi noise in capture stream prevents from using the videophone in speakerphone mode, but in normal (handset) mode, the noise level seems still acceptable. The only missing piece of a puzzle is my patch, that prevents the kernel driver from putting corrupted values into the McBSP control registers. That learned, please reconsider: 1. Can my patch break anything, related or not? 2. How does it affect performance of otherwise unaffected machines? 3. Is correcting a poorly designed machine behaviour worth of the change? Thanks, Janusz ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-09-04 10:34 ` Janusz Krzysztofik @ 2009-09-04 11:07 ` Eero Nurkkala 2009-09-05 17:31 ` Anuj Aggarwal 0 siblings, 1 reply; 15+ messages in thread From: Eero Nurkkala @ 2009-09-04 11:07 UTC (permalink / raw) To: ext Janusz Krzysztofik; +Cc: linux-omap@vger.kernel.org, e3-hacking@earth.li On Fri, 2009-09-04 at 12:34 +0200, ext Janusz Krzysztofik wrote: > in normal (handset) mode, the noise level seems still acceptable. The only > missing piece of a puzzle is my patch, that prevents the kernel driver from > putting corrupted values into the McBSP control registers. > > That learned, please reconsider: > 1. Can my patch break anything, related or not? That needs to be investigated in more detail. > 2. How does it affect performance of otherwise unaffected machines? Like you already know, shouldn't. > 3. Is correcting a poorly designed machine behaviour worth of the change? > > Thanks, > Janusz I'd say this is in the right track. All McBSP registers (not status etc) will need to be stored in memory. (Now, they're not). And all those register contents will need to be written back in certain situations. So this is the case when there is an external audio chip, that takes audio in in bursts. So once the burst is complete, all McBSP clocks will be disabled for a duration. Then (at least >= 3430) device will hit OFF mode, meaning all register contents are lost, also McBSP's. They will need to be written back at some point. My quick verdict is, that your patch is, going into right direction. But the thing is that my words don't count much ;) (Possibly worth taking the patch in, if guaranteed to not break any others). - Eero ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-09-04 11:07 ` Eero Nurkkala @ 2009-09-05 17:31 ` Anuj Aggarwal 2009-09-05 20:46 ` ext-Eero.Nurkkala 0 siblings, 1 reply; 15+ messages in thread From: Anuj Aggarwal @ 2009-09-05 17:31 UTC (permalink / raw) To: ext-eero.nurkkala Cc: ext Janusz Krzysztofik, linux-omap@vger.kernel.org, e3-hacking@earth.li On Fri, Sep 4, 2009 at 4:37 PM, Eero Nurkkala<ext-eero.nurkkala@nokia.com> wrote: > On Fri, 2009-09-04 at 12:34 +0200, ext Janusz Krzysztofik wrote: >> in normal (handset) mode, the noise level seems still acceptable. The only >> missing piece of a puzzle is my patch, that prevents the kernel driver from >> putting corrupted values into the McBSP control registers. >> >> That learned, please reconsider: >> 1. Can my patch break anything, related or not? > > That needs to be investigated in more detail. > >> 2. How does it affect performance of otherwise unaffected machines? > > Like you already know, shouldn't. > >> 3. Is correcting a poorly designed machine behaviour worth of the change? >> >> Thanks, >> Janusz > > > I'd say this is in the right track. All McBSP registers > (not status etc) will need to be stored in memory. (Now, they're not). > And all those register contents will need to be written back > in certain situations. So this is the case when there is an external > audio chip, that takes audio in in bursts. So once the > burst is complete, all McBSP clocks will be disabled for a duration. > > Then (at least >= 3430) device will hit OFF mode, meaning > all register contents are lost, also McBSP's. They will > need to be written back at some point. Isn't the better way to achieve this thing, at least from the OFF mode perspective, is to provide similar implementation for McBSP in omap3_core_save_context () / omap3_core_restore_context(), in file arch/arm/mach-omap2/pm34xx.c? Code for other core peripherals is already there and we have to add support for rest of the core-domain peripherals. > > My quick verdict is, that your patch is, going into right > direction. But the thing is that my words don't count much ;) > > (Possibly worth taking the patch in, if guaranteed to not break > any others). > > - Eero > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Best Regards, Anuj Aggarwal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC][PATCH] ARM: OMAP: McBSP: Use register cache 2009-09-05 17:31 ` Anuj Aggarwal @ 2009-09-05 20:46 ` ext-Eero.Nurkkala 0 siblings, 0 replies; 15+ messages in thread From: ext-Eero.Nurkkala @ 2009-09-05 20:46 UTC (permalink / raw) To: anuj.aggarwal; +Cc: jkrzyszt, linux-omap, e3-hacking > Isn't the better way to achieve this thing, at least from the OFF mode > perspective, is to provide similar implementation for McBSP in > omap3_core_save_context () / omap3_core_restore_context(), in > file arch/arm/mach-omap2/pm34xx.c? Code for other core peripherals > is already there and we have to add support for rest of the core-domain > peripherals. Yes and no. I'm not sure McBSP context would be needed to be restored after every wake up from PER off mode.. isn't that how it's handled now for other peripherals? But maybe restore context for McBSP whenever there's need for it. Rewriting millions of registers just for the case doesn't sound good. IMO, with an irq from an external device, a rewrite of the McBSP registers occurs. You loose the point of the OFF mode if you always start rewriting all registers? (you're not saving power.. the opposite). Or did I not understand something? (most likely the case) =) - Eero ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2009-11-28 16:13 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-12 10:39 [RFC][PATCH] ARM: OMAP: McBSP: Use register cache Janusz Krzysztofik 2009-08-28 11:26 ` Janusz Krzysztofik 2009-11-14 2:06 ` Janusz Krzysztofik 2009-11-23 20:42 ` Janusz Krzysztofik 2009-11-23 23:53 ` Tony Lindgren 2009-11-24 7:59 ` Jarkko Nikula 2009-11-24 11:31 ` [RFC][PATCH v2] " Janusz Krzysztofik 2009-11-26 8:00 ` Jarkko Nikula 2009-11-26 13:25 ` Janusz Krzysztofik 2009-11-28 16:14 ` Jarkko Nikula -- strict thread matches above, loose matches on Subject: below -- 2009-09-04 5:39 [RFC][PATCH] ARM: " Eero Nurkkala 2009-09-04 10:34 ` Janusz Krzysztofik 2009-09-04 11:07 ` Eero Nurkkala 2009-09-05 17:31 ` Anuj Aggarwal 2009-09-05 20:46 ` ext-Eero.Nurkkala
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox