Hello Janusz,
On Friday 15 January 2010 18:43:11 ext Janusz Krzysztofik wrote:
> >> --- git/arch/arm/plat-omap/mcbsp.c.orig 2010-01-14 00:36:14.000000000
> >> +0100 +++ git/arch/arm/plat-omap/mcbsp.c 2010-01-14 02:05:23.000000000
> >> +0100 @@ -114,7 +114,8 @@ static irqreturn_t omap_mcbsp_tx_irq_han
> >> dev_err(mcbsp_tx->dev, "TX Frame Sync Error! : 0x%x\n",
> >> irqst_spcr2);
> >> /* Writing zero to XSYNC_ERR clears the IRQ */
> >> - MCBSP_WRITE(mcbsp_tx, SPCR2, irqst_spcr2 & ~(XSYNC_ERR));
> >> + MCBSP_WRITE(mcbsp_tx, SPCR2,
> >> + MCBSP_READ_CACHE(mcbsp_tx, SPCR2) & ~(XSYNC_ERR));
> >
> > The reg_cache will never have the XSYNC_ERR, or any other flags set,
> > since these flags has never written to the reg_cache.
> > So it is kind of not necessary to clear the flag, which is actually
> > always 0.
>
> Agree.
>
> > Another thing is that as far as I understand the reason behind of this
> > series is that you have a problem, that you can not trust on the values
> > that you read from the McBSP registers, if this is true, than the problem
> > can occur when the above path has been taken:
> >
> > ...
> > irqst_spcr2 = MCBSP_READ(mcbsp_tx, SPCR2);
> > ...
> > if (irqst_spcr2 & XSYNC_ERR) {
> >
> > But since you read from McBSP registers much rarely, than probably the
> > corruption is not that visible?
>
> Sure no software solution can correct my hardware issue in case of
> register bits manintained by the hardware itself. Well, maybe software
> that limits heat dissipation by lowering overal power consumption could
> do to some extent ;).
>
> What I'm going to address here is only a case when writing back possibly
> corrupted bits can be avoided if correct values are well known and
> can be determined without reading them back from the register itself.
Yeah, this is also my understanding, but I just did not found the right words ;)
>
> > Anyway, clearing the status/error flags are not necessary, since the
> > reg_cache will never got these bits set, you could just write back the
> > SPCR2/SPCR1 register content from the cache as it is...
> >
> >> } else {
> >> complete(&mcbsp_tx->tx_irq_completion);
> >> }
> >> @@ -134,7 +135,8 @@ static irqreturn_t omap_mcbsp_rx_irq_han
> >> dev_err(mcbsp_rx->dev, "RX Frame Sync Error! : 0x%x\n",
> >> irqst_spcr1);
> >> /* Writing zero to RSYNC_ERR clears the IRQ */
> >> - MCBSP_WRITE(mcbsp_rx, SPCR1, irqst_spcr1 & ~(RSYNC_ERR));
> >> + MCBSP_WRITE(mcbsp_rx, SPCR1,
> >> + MCBSP_READ_CACHE(mcbsp_rx, SPCR1) & ~(RSYNC_ERR));
> >
> > Same here.
> >
> > ...
> >
> >> @@ -653,7 +657,7 @@ int omap_mcbsp_pollwrite(unsigned int id
> >> if (MCBSP_READ(mcbsp, SPCR2) & XSYNC_ERR) {
> >> /* clear error */
> >> MCBSP_WRITE(mcbsp, SPCR2,
> >> - MCBSP_READ(mcbsp, SPCR2) & (~XSYNC_ERR));
> >> + MCBSP_READ_CACHE(mcbsp, SPCR2) & (~XSYNC_ERR));
> >
> > Well, I think here also, the reg_cache does not have the XSYNC_ERR set,
> > it is only set in the McBSP register.
> >
> >> /* resend */
> >> return -1;
> >> } else {
> >> @@ -662,10 +666,12 @@ int omap_mcbsp_pollwrite(unsigned int id
> >> while (!(MCBSP_READ(mcbsp, SPCR2) & XRDY)) {
> >> if (attemps++ > 1000) {
> >> MCBSP_WRITE(mcbsp, SPCR2,
> >> - MCBSP_READ(mcbsp, SPCR2) & (~XRST));
> >> + MCBSP_READ_CACHE(mcbsp, SPCR2) &
> >> + (~XRST));
> >
> > Also here, the XRST will surely not set in the cached SPCR2...
> >
> > This applies fro all other cases regarding to status/error bits in McBSP.
>
> OK, I can try to identify all those cases.
>
> What about introducing this simplification in a separate followup patch,
> quoting your rationale in its changelog? I can try to prepare one if you
> agree.
I think it is OK to have a followup patch addressing these.
Just mention in a comment, that you are writing the cached value back to the
register, which does not have these status flags set, thus clearing the reason
in McBSP.
Jarkko: What do you think?
Otherwise, I think this set is good to go.
>
> Thanks,
> Janusz
>
--
Péter
--
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
