On 15-03-30 11:42 PM, Mark Brown wrote:
> On Mon, Mar 30, 2015 at 08:16:24PM -0700, Scott Branden wrote:
>
> The CC list for this patch is pretty wide - please look at who you're
> sending this to and try to send to only relevant people (for example I'm
> not sure the Raspberry Pi people need to review this). People working
> upstream get a lot of mail so it's helpful to avoid filling their
> inboxes with random irrelevant stuff.
>
>> sound/soc/bcm/Kconfig | 11 +
>> sound/soc/bcm/Makefile | 5 +-
>> sound/soc/bcm/cygnus-pcm.c | 918 +++++++++++++++++++++++++
>> sound/soc/bcm/cygnus-pcm.h | 45 ++
>> sound/soc/bcm/cygnus-ssp.c | 1613 ++++++++++++++++++++++++++++++++++++++++++++
>> sound/soc/bcm/cygnus-ssp.h | 84 +++
>> 6 files changed, 2675 insertions(+), 1 deletion(-)
>
> Send the DMA and DAI drivers as separate patches please, it makes review
> easier.
>
>> +config SND_SOC_CYGNUS
>> + tristate "SoC platform audio for Broadcom Cygnus chips"
>> + depends on ARCH_BCM_CYGNUS || COMPILE_TEST
>> + default ARCH_BCM_CYGNUS
>
Okay.
> Remove the default setting here - we don't do this for other drivers, we
> shouldn't do it here.
>
>> +/*
>> + * PERIOD_BYTES_MIN is the number of bytes to at which the interrupt will tick.
>> + * This number should be a multiple of 256
>> + */
>> +#define PERIOD_BYTES_MIN 0x100
>
> This sounds like it's a setting rather than actually the minimum?
>
It is a bad comment. I will update. This is the minimum period (in
bytes) at which the interrupt can tick. Other possible value for the
period must be multiple of this value.
>> +static const struct snd_pcm_hardware cygnus_pcm_hw = {
>> + .info = SNDRV_PCM_INFO_MMAP |
>> + SNDRV_PCM_INFO_MMAP_VALID |
>> + SNDRV_PCM_INFO_INTERLEAVED,
>
> The DMA controller is integrated into the IP?
>
Yes, it is dedicated for the audio driver.
>> +static int enable_count;
>
> This looks bogus - why is this a global variable not part of the device
> struct and if it does need to be global why does it need no locking?
>
I will fix.
>> + if (aio->portnum == 0)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(0);
>> + else if (aio->portnum == 1)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(2);
>> + else if (aio->portnum == 2)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(4);
>> + else if (aio->portnum == 3)
>> + *p_rbuf = RINGBUF_REG_PLAYBACK(6); /*SPDIF */
>> + else
>> + status = -EINVAL;
>
> This looks like a switch statement, there are many places in the code
> where you're writing switch statements as chains of ifs.
>
No problem. Will use switch statements.
>> +static void ringbuf_inc(void __iomem *audio_io, bool is_playback,
>> + const struct ringbuf_regs *p_rbuf)
>> +{
>> + u32 regval, endval, active_ptr;
>> +
>> + if (is_playback)
>> + active_ptr = p_rbuf->wraddr;
>> + else
>> + active_ptr = p_rbuf->rdaddr;
>> +
>> + endval = readl(audio_io + p_rbuf->endaddr);
>> + regval = readl(audio_io + active_ptr);
>> + regval = regval + p_rbuf->period_bytes;
>> + if (regval > endval)
>> + regval -= p_rbuf->buf_size;
>> +
>> + writel(regval, audio_io + active_ptr);
>> +}
>
> So it looks like we're getting an interrupt per period and we have to
> manually advance to the next one?
>
Yes.
>> +static irqreturn_t cygnus_dma_irq(int irq, void *data)
>> +{
>> + u32 r5_status;
>> + struct cygnus_audio *cygaud;
>> +
>> + cygaud = (struct cygnus_audio *)data;
>
> If you need to cast away from void something is very wrong.
>
Okay, will fix.
>> + /*
>> + * R5 status bits Description
>> + * 0 ESR0 (playback FIFO interrupt)
>> + * 1 ESR1 (playback rbuf interrupt)
>> + * 2 ESR2 (capture rbuf interrupt)
>> + * 3 ESR3 (Freemark play. interrupt)
>> + * 4 ESR4 (Fullmark capt. interrupt)
>> + */
>> + r5_status = readl(cygaud->audio + INTH_R5F_STATUS_OFFSET);
>> +
>> + /* If playback interrupt happened */
>> + if (ANY_PLAYBACK_IRQ & r5_status)
>> + handle_playback_irq(cygaud);
>> +
>> + /* If capture interrupt happened */
>> + if (ANY_CAPTURE_IRQ & r5_status)
>> + handle_capture_irq(cygaud);
>> +
>> + /*
>> + * clear r5 interrupts after servicing them to avoid overwriting
>> + * esr_status
>> + */
>> + writel(r5_status, cygaud->audio + INTH_R5F_CLEAR_OFFSET);
>
> This feels racy especially given that we seem to need every interrupt
> delivering. What if another period happens during the servicing? I
> don't understand what "overwriting esr_status" means here.
>
Let me review this handler and I will enhance as needed.
>> + return IRQ_HANDLED;
>
> If neither playback nor capture interrupts were flagged we should return
> IRQ_NONE.
>
Okay, will fix.
>> +/*
>> + * This code is identical to what is done by the framework, when we do not
>> + * supply a 'copy' function. Having our own copy hook in place allows for
>> + * us to easily add some diagnotics when needed.
>> + */
>> +int cygnus_pcm_copy(struct snd_pcm_substream *substream, int channel,
>> + snd_pcm_uframes_t pos,
>> + void __user *buf, snd_pcm_uframes_t count)
>
> This is obviously not suitable for mainline - "let's just cut'n'paste
> the shared code in case we want to add diagnostics in future" does not
> scale, you can always add local diagnostics in the core code.
>
Okay, will remove.
>> +};
>> +/*
>
> Blank line between these two please. Missing blanks between bits of
> code seem to be a common issue in this driver.
>
Okay, will fix.
>> +static int audio_ssp_in_enable(struct cygnus_aio_port *aio, bool enable)
>> +{
>> + u32 value;
>> +
>> + if (enable) {
>
>> + } else {
>
> Why not just write two functions?
>
Okay, will change.
>> +static int audio_ssp_source_bitres(struct cygnus_aio_port *aio, unsigned bits)
>> +{
>> + u32 mask = 0x1f;
>> + u32 value = 0;
>> +
>> + if ((bits == 8) || (bits == 16) || (bits == 32)) {
>> + value = readl(aio->audio + aio->regs.bf_sourcech_cfg);
>> + value &= ~(mask << BF_SRC_CFGX_BIT_RES);
>> +
>> + /* 32 bit mode is coded as 0 */
>> + if ((bits == 8) || (bits == 16))
>> + value |= (bits << BF_SRC_CFGX_BIT_RES);
>> +
>> + writel(value, aio->audio + aio->regs.bf_sourcech_cfg);
>> + return 0;
>> + }
>> +
>> + pr_err("Bit resolution not supported %d\n", bits);
>> + return -EINVAL;
>
> dev_err() (this applies throughout the driver).
>
Okay, will convert pr_err to dev_err.
> It's not clear why this is a function and not just written as a single
> statement in the one place that it's used (there are multiple calls but
> they're all together and could just be inlined).
>
I can integrate it with the calling code.
>> + if (!aio->bitrate) {
>> + pr_err("%s Use .set_clkdiv() to set bitrate\n", __func__);
>> + return -EINVAL;
>> + }
>
> This seems bogus - why are we enforcing the use of set_clkdiv()? Can't
> we figure out something esneible?
>
Yes, I believe I can set this via "set_tdm_slot".
>> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> + /* Set the SSP up as slave */
>> + case SND_SOC_DAIFMT_CBM_CFM:
>
> The comments here look odd due to the indentation and really aren't
> adding much anyway.
>
Okay, will remove.
>> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>> + case SND_SOC_DAIFMT_RIGHT_J:
>> + case SND_SOC_DAIFMT_LEFT_J:
>> + return -EINVAL;
>
> These are just eaten by the default case.
>
Okay, will remove.
>> + case SND_SOC_DAIFMT_DSP_A:
>> + case SND_SOC_DAIFMT_DSP_B:
>> + ssp_newcfg |= BIT(I2S_OUT_CFGX_TDM_MODE);
>> +
>> + /* DSP_A = data after FS, DSP_B = data during FS */
>> + if (SND_SOC_DAIFMT_DSP_A)
>> + ssp_newcfg |= BIT(I2S_OUT_CFGX_DATA_ALIGNMENT);
>
> That if statement isn't doing what was intended...
>
Yikes. I will fix that.
>> + } else {
>> +
>> + switch (cmd) {
>> + case SNDRV_PCM_TRIGGER_START:
>> + audio_ssp_in_enable(aio, 1);
>> + break;
>> +
>> + case SNDRV_PCM_TRIGGER_STOP:
>> + audio_ssp_in_enable(aio, 0);
>> + break;
>> + }
>
> We can just ignore other triggers? It's not clear why this is different
> to the playback case.
>
I will review this behaviour and fix it up.
>> +int cygnus_ssp_get_mode(struct snd_soc_dai *cpu_dai)
>> +{
>> + struct cygnus_aio_port *aio = cygnus_dai_get_portinfo(cpu_dai);
>> +
>> + return aio->mode;
>> +}
>> +EXPORT_SYMBOL(cygnus_ssp_get_mode);
>
> What is this for, it's setting off alarm bells? Note also that ASoC is
> _GPL() only.
>
Okay, will remove. It is not needed if I set the port mode from the
machine file (current done via device tree).
>> +static const struct snd_kcontrol_new pll_tweak_controls[] = {
>> + SOC_SINGLE_EXT("PLL Tweak", 0, 0, PLL_NDIV_FRACT_MAX, 0,
>> + pll_tweak_get, pll_tweak_put),
>> +};
>
> Whatever this control is doing it should be a separate patch (as I think
> we discussed in person a ELC?), it's clearly something that's unusual
> and is likely to block the rest of the code as a result. At a high
> level this needs at least some documentation.
>
Okay, will remove.
>> +int cygnus_ssp_add_pll_tweak_controls(struct snd_soc_pcm_runtime *rtd)
>> +{
>> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +
>> + return snd_soc_add_dai_controls(cpu_dai,
>> + pll_tweak_controls,
>> + ARRAY_SIZE(pll_tweak_controls));
>> +}
>> +EXPORT_SYMBOL(cygnus_ssp_add_pll_tweak_controls);
>
> Again, why is this being exported and why is it not _GPL? If the driver
> is adding controls I'd expect it to just add its controls itself.
>
Okay, will remove.
>> +static int cygnus_ssp_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *child_node;
>> + struct resource *res = pdev->resource;
>> + struct cygnus_audio *cygaud;
>> + int err = -EINVAL;
>> + int node_count;
>> + int active_port_count;
>> +
>> + if (!of_match_device(cygnus_ssp_of_match, dev)) {
>> + dev_err(dev, "Failed to find ssp controller\n");
>> + return -ENODEV;
>> + }
>
> This error message is misleading - you mean to say that the driver got
> loaded for a device it doesn't understand.
>
Okay, will remove.
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + cygaud->audio = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(cygaud->audio)) {
>> + dev_err(dev, "audio_io ioremap failed\n");
>> + return PTR_ERR(cygaud->audio);
>
> devm_ioremap_resource() will complain for you, and in general you should
> be printing error codes.
>
Okay. Will remove and rely on devm_ipremap message.
>> + node_count = 0;
>
> This doesn't seem to be needed?
>
Okay, will clean up.
>> + node_count = of_get_child_count(pdev->dev.of_node);
>> + if ((node_count < 1) || (node_count > CYGNUS_MAX_PORTS)) {
>> + dev_err(dev, "Incorrct number of child nodes\n");
>> + return -EINVAL;
>
> Spelling mistake there and it would be helpful to the user to tell them
> what we parsed.
>
Okay, will fix.
On Thu, Apr 02, 2015 at 11:47:18AM -0700, Lori Hikichi wrote:
> On 15-03-30 11:42 PM, Mark Brown wrote:
> >>+config SND_SOC_CYGNUS
> >>+ tristate "SoC platform audio for Broadcom Cygnus chips"
> >>+ depends on ARCH_BCM_CYGNUS || COMPILE_TEST
> >>+ default ARCH_BCM_CYGNUS
> Okay.
You don't need to reply to every single comment, the general assumption
will be that if there's no other followup all review comments will be
addressed. It's better to just reply to things where there's something
more detailed to say, if you explicitly reply to everything then that
makes it easier for actual replies to be missed since there's a lot of
there's a lot of the mail that's just going to be skipped through.
> >>+static void ringbuf_inc(void __iomem *audio_io, bool is_playback,
> >>+ const struct ringbuf_regs *p_rbuf)
> >So it looks like we're getting an interrupt per period and we have to
> >manually advance to the next one?
> Yes.
OK, so that seems fragile - what happens if we're slightly late
processing an interrupt or miss one entirely? Most hardware has some
way to read back the current position which tends to be more reliable,
is that not an option here?
On 15-04-06 09:19 AM, Mark Brown wrote:
> On Thu, Apr 02, 2015 at 11:47:18AM -0700, Lori Hikichi wrote:
>> On 15-03-30 11:42 PM, Mark Brown wrote:
>
>>>> +config SND_SOC_CYGNUS
>>>> + tristate "SoC platform audio for Broadcom Cygnus chips"
>>>> + depends on ARCH_BCM_CYGNUS || COMPILE_TEST
>>>> + default ARCH_BCM_CYGNUS
>
>> Okay.
>
> You don't need to reply to every single comment, the general assumption
> will be that if there's no other followup all review comments will be
> addressed. It's better to just reply to things where there's something
> more detailed to say, if you explicitly reply to everything then that
> makes it easier for actual replies to be missed since there's a lot of
> there's a lot of the mail that's just going to be skipped through.
>
>>>> +static void ringbuf_inc(void __iomem *audio_io, bool is_playback,
>>>> + const struct ringbuf_regs *p_rbuf)
>
>>> So it looks like we're getting an interrupt per period and we have to
>>> manually advance to the next one?
>
>> Yes.
>
> OK, so that seems fragile - what happens if we're slightly late
> processing an interrupt or miss one entirely? Most hardware has some
> way to read back the current position which tends to be more reliable,
> is that not an option here?
>
The hardware updates a read pointer (rdaddr) which we feed to ALSA via
the ".pointer" hook. So yes, the hardware does have a register that
tells us its progress. This routine (ringbuf_inc) actually updates a
write pointer (wraddr) which is a misnomer. The write pointer doesn?t
actually tell us where we are writing to ? ALSA keeps track of that.
The wraddr only prevents the hardware from reading past it. We just use
it, along with a low water mark configuration register, to keep the
periodic interrupts firing. The hardware is tracking the distance
between rdaddr and wraddr and comparing that to the low water mark.
Being late processing the interrupt is okay since there are more samples
to read. That is, it was only a low water mark interrupt and we have
one period of valid data still (we configure low water to be one
period). Missing an interrupt is okay since the hardware will just stop
reading from the ring buffer when rdaddr has hit wraddr.
On Tue, Apr 07, 2015 at 07:30:57PM -0700, Lori Hikichi wrote:
> On 15-04-06 09:19 AM, Mark Brown wrote:
> >OK, so that seems fragile - what happens if we're slightly late
> >processing an interrupt or miss one entirely? Most hardware has some
> >way to read back the current position which tends to be more reliable,
> >is that not an option here?
> The hardware updates a read pointer (rdaddr) which we feed to ALSA via the
> ".pointer" hook. So yes, the hardware does have a register that tells us its
> progress. This routine (ringbuf_inc) actually updates a write pointer
> (wraddr) which is a misnomer. The write pointer doesn’t actually tell us
> where we are writing to – ALSA keeps track of that. The wraddr only prevents
> the hardware from reading past it. We just use it, along with a low water
> mark configuration register, to keep the periodic interrupts firing. The
> hardware is tracking the distance between rdaddr and wraddr and comparing
> that to the low water mark.
The code has handling for both read and write so it's not just updating
a write pointer. Is there no flexibility in the hardware regarding
interrupt generation?
> Being late processing the interrupt is okay since there are more samples to
> read. That is, it was only a low water mark interrupt and we have one
> period of valid data still (we configure low water to be one period).
> Missing an interrupt is okay since the hardware will just stop reading from
> buffer when rdaddr has hit wraddr.
Stopping if we miss an interrupt is precisely the sort of situation we
want to avoid if we can - if the application is sufficiently far ahead
of the hardware everything should continue to work fine. The minimal
period size appears to be very small so this is a potential issue, if an
application tries to use many very small periods it's both more
vulnerable to some other interrupt taking longer than might be desirable
and likely that things would be fine as the application is hopefully
more than one period ahead of where the hardware is at.
If the hardware is always going to halt at the end of the period there's
not a huge amount we can do about this except possibly raise the minimum
period if systems are running into trouble but if there's a way to avoid
doing that then that would be even better.
On 15-04-08 12:23 PM, Mark Brown wrote:
> On Tue, Apr 07, 2015 at 07:30:57PM -0700, Lori Hikichi wrote:
>> On 15-04-06 09:19 AM, Mark Brown wrote:
>
>>> OK, so that seems fragile - what happens if we're slightly late
>>> processing an interrupt or miss one entirely? Most hardware has some
>>> way to read back the current position which tends to be more reliable,
>>> is that not an option here?
>
>> The hardware updates a read pointer (rdaddr) which we feed to ALSA via the
>> ".pointer" hook. So yes, the hardware does have a register that tells us its
>> progress. This routine (ringbuf_inc) actually updates a write pointer
>> (wraddr) which is a misnomer. The write pointer doesn’t actually tell us
>> where we are writing to – ALSA keeps track of that. The wraddr only prevents
>> the hardware from reading past it. We just use it, along with a low water
>> mark configuration register, to keep the periodic interrupts firing. The
>> hardware is tracking the distance between rdaddr and wraddr and comparing
>> that to the low water mark.
>
> The code has handling for both read and write so it's not just updating
> a write pointer. Is there no flexibility in the hardware regarding
> interrupt generation?
>
>> Being late processing the interrupt is okay since there are more samples to
>> read. That is, it was only a low water mark interrupt and we have one
>> period of valid data still (we configure low water to be one period).
>> Missing an interrupt is okay since the hardware will just stop reading from
>> buffer when rdaddr has hit wraddr.
>
> Stopping if we miss an interrupt is precisely the sort of situation we
> want to avoid if we can - if the application is sufficiently far ahead
> of the hardware everything should continue to work fine. The minimal
> period size appears to be very small so this is a potential issue, if an
> application tries to use many very small periods it's both more
> vulnerable to some other interrupt taking longer than might be desirable
> and likely that things would be fine as the application is hopefully
> more than one period ahead of where the hardware is at.
>
> If the hardware is always going to halt at the end of the period there's
> not a huge amount we can do about this except possibly raise the minimum
> period if systems are running into trouble but if there's a way to avoid
> doing that then that would be even better.
>
Makes sense, thanks for clarifying the desired behaviour, I will make the
necessary adjustments to keep the hardware supplied with valid data even if
interrupts are held off past a whole period.