Thursday 26 November 2009 09:00:00 Jarkko Nikula napisał(a):
> Hi
>
> On Tue, 24 Nov 2009 12:31:16 +0100
>
> Janusz Krzysztofik <jkrzyszt@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
