Re: [PATCH 4/4] pxa-ssp: switch from network mode to psp

[Daniel Mack <daniel@caiaq.de>]

Hi Philipp,

On Wed, Mar 04, 2009 at 09:56:01PM +0100, pHilipp Zabel wrote:
> > diff --git a/sound/soc/pxa/pxa-ssp.c b/sound/soc/pxa/pxa-ssp.c
> > index 45fb600..97f11d6 100644
> > --- a/sound/soc/pxa/pxa-ssp.c
> > +++ b/sound/soc/pxa/pxa-ssp.c
> > @@ -319,6 +319,7 @@ static int pxa_ssp_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
> >                break;
> >        case PXA_SSP_CLK_EXT:
> >                priv->sysclk = freq;
> > +               ssp_set_scr(&priv->dev, 4);
> 
> Shouldn't this be somehow set by set_dai_clkdiv instead?

This is actually a very common case - MCLK is BCLK*4, so I put it here.
We could still move it to some more configurable place if it turns out
we need to configure it per board.

> >                switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> >                case SND_SOC_DAIFMT_NB_NF:
> > -                       sspsp |= SSPSP_FSRT;
> >                        break;
> >                case SND_SOC_DAIFMT_NB_IF:
> > -                       sspsp |= SSPSP_SFRMP | SSPSP_FSRT;
> > -                       break;
> > -               case SND_SOC_DAIFMT_IB_IF:
> >                        sspsp |= SSPSP_SFRMP;
> >                        break;
> 
> Removal of SSPSP_FSRT from NB/IB selection seems to be correct from the docs.

SSPSP_FSRT has a totally different meaning according to the PXA3xx docs,
but I'll have a look at the PXA2x specs - maybe we need a special case
here. Unfortunately, I'm not able to quote from PXA3x specs here due to
NDA restrictions.

> Can you check if IB could be properly handled by setting SCMODE(1)?

Can't follow that - what are you referring to here?

> >                default:
> > @@ -652,33 +649,39 @@ static int pxa_ssp_hw_params(struct snd_pcm_substream *substream,
> >                break;
> >        case SNDRV_PCM_FORMAT_S24_LE:
> >                sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(8));
> > -               /* we must be in network mode (2 slots) for 24 bit stereo */
> 
> This is still dubious ...
> S24_LE is 24-bit sound LSB-aligned in 32-bit frames, so DataSize
> should be 32 here.

I didn't test that, and I didn't change it either - I just removed the
comment, as we're not running in network mode anymore.

> >                break;
> >        case SNDRV_PCM_FORMAT_S32_LE:
> >                sscr0 |= (SSCR0_EDSS | SSCR0_DataSize(16));
> > -               /* we must be in network mode (2 slots) for 32 bit stereo */
> 
> How is it possible to send 64bit in one frame otherwise?

We're not running in network mode anymore - things are different now :)

> > +               switch (priv->dai_fmt & SND_SOC_DAIFMT_FRAME_FORMAT_MASK) {
> > +               case SND_SOC_DAIFMT_FF_I2S_32:
> > +                       /* These values are all found out by trying and
> > +                        * failing a lot. PXA's SSP is all black magic and
> > +                        * does not work like described in any datasheet.
> > +                        */
> > +                       sspsp |= SSPSP_SFRMWDTH(32);
> > +                       sspsp |= SSPSP_SFRMDLY(32 * 2);
> > +                       sspsp |= SSPSP_EDMYSTOP(3);
> > +                       sspsp |= SSPSP_DMYSTOP(3);
> > +                       sspsp |= SSPSP_DMYSTRT(1);
> 
> Wha?! Amazing. And this really works?
> How the hell can this result in 16 bits of data followed by 16 bits of
> zeroes, twice :)

Don't ask :) If we could explain in detail what these registers mean,
I'd rather make them macro values, but unfortunately, the comment is
correct ...

> 
> > +                       break;
> > +               default:
> > +                       /* Cleared when the DAI format is set */
> > +                       sspsp |= SSPSP_SFRMWDTH(width);
> 
> Not good for DSP_A/B.

This is the SND_SOC_DAIFMT_I2S case, so DSP modes will be unaffected.

Could you try the patches on your board?

Thanks,
Daniel