Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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? signature.asc Description: Digital signature
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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? signature.asc Description: Digital signature
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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.
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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
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?
> +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?
> +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?
> + 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.
> +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?
> +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.
> + /*
> + * 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.
> + return IRQ_HANDLED;
If neither playback nor capture interrupts were flagged we should return
IRQ_NONE.
> +/*
> + * 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
Re: [PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
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
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?
+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?
+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?
+ 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.
+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?
+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.
+ /*
+ * 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.
+ return IRQ_HANDLED;
If neither playback nor capture interrupts were flagged we should return
IRQ_NONE.
+/*
+ * 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.
+};
+/*
Blank line between
[PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
From: Lori Hikichi The audio block has 4 serial ports. 3 ports are configurable as either I2S or TDM. The 4th port is for SPDIF transmit. This audio block is found on the bcm958305, bcm958300, and bcm911360. Reviewed-by: Jonathan Richardson Signed-off-by: Lori Hikichi Signed-off-by: Scott Branden --- 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(-) create mode 100644 sound/soc/bcm/cygnus-pcm.c create mode 100644 sound/soc/bcm/cygnus-pcm.h create mode 100644 sound/soc/bcm/cygnus-ssp.c create mode 100644 sound/soc/bcm/cygnus-ssp.h diff --git a/sound/soc/bcm/Kconfig b/sound/soc/bcm/Kconfig index 6a834e1..2c5ff37 100644 --- a/sound/soc/bcm/Kconfig +++ b/sound/soc/bcm/Kconfig @@ -7,3 +7,14 @@ config SND_BCM2835_SOC_I2S Say Y or M if you want to add support for codecs attached to the BCM2835 I2S interface. You will also need to select the audio interfaces to support below. + +config SND_SOC_CYGNUS + tristate "SoC platform audio for Broadcom Cygnus chips" + depends on ARCH_BCM_CYGNUS || COMPILE_TEST + default ARCH_BCM_CYGNUS + help + Say Y if you want to add support for ASoC audio on Broadcom + Cygnus chips (bcm958300, bcm958305, bcm911360) + + If you don't know what to do here, say N. + diff --git a/sound/soc/bcm/Makefile b/sound/soc/bcm/Makefile index bc816b7..5c39790 100644 --- a/sound/soc/bcm/Makefile +++ b/sound/soc/bcm/Makefile @@ -1,5 +1,8 @@ # BCM2835 Platform Support snd-soc-bcm2835-i2s-objs := bcm2835-i2s.o -obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o +# CYGNUS Platform Support +snd-soc-cygnus-objs := cygnus-pcm.o cygnus-ssp.o +obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o +obj-$(CONFIG_SND_SOC_CYGNUS) += snd-soc-cygnus.o diff --git a/sound/soc/bcm/cygnus-pcm.c b/sound/soc/bcm/cygnus-pcm.c new file mode 100644 index 000..3a4106b --- /dev/null +++ b/sound/soc/bcm/cygnus-pcm.c @@ -0,0 +1,918 @@ +/* + * Copyright (C) 2014-2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "cygnus-ssp.h" +#include "cygnus-pcm.h" + +/* Register offset needed for ASoC PCM module */ + +#define INTH_R5F_STATUS_OFFSET 0x040 +#define INTH_R5F_CLEAR_OFFSET 0x048 +#define INTH_R5F_MASK_SET_OFFSET 0x050 +#define INTH_R5F_MASK_CLEAR_OFFSET 0x054 + +#define BF_REARM_FREE_MARK_OFFSET 0x344 +#define BF_REARM_FULL_MARK_OFFSET 0x348 + +/* Ring Buffer Ctrl Regs --- Start */ +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_RDADDR_REG_BASE */ +#define SRC_RBUF_0_RDADDR_OFFSET 0x500 +#define SRC_RBUF_1_RDADDR_OFFSET 0x518 +#define SRC_RBUF_2_RDADDR_OFFSET 0x530 +#define SRC_RBUF_3_RDADDR_OFFSET 0x548 +#define SRC_RBUF_4_RDADDR_OFFSET 0x560 +#define SRC_RBUF_5_RDADDR_OFFSET 0x578 +#define SRC_RBUF_6_RDADDR_OFFSET 0x590 + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_WRADDR_REG_BASE */ +#define SRC_RBUF_0_WRADDR_OFFSET 0x504 +#define SRC_RBUF_1_WRADDR_OFFSET 0x51c +#define SRC_RBUF_2_WRADDR_OFFSET 0x534 +#define SRC_RBUF_3_WRADDR_OFFSET 0x54c +#define SRC_RBUF_4_WRADDR_OFFSET 0x564 +#define SRC_RBUF_5_WRADDR_OFFSET 0x57c +#define SRC_RBUF_6_WRADDR_OFFSET 0x594 + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_BASEADDR_REG_BASE */ +#define SRC_RBUF_0_BASEADDR_OFFSET 0x508 +#define SRC_RBUF_1_BASEADDR_OFFSET 0x520 +#define SRC_RBUF_2_BASEADDR_OFFSET 0x538 +#define SRC_RBUF_3_BASEADDR_OFFSET 0x550 +#define SRC_RBUF_4_BASEADDR_OFFSET 0x568 +#define SRC_RBUF_5_BASEADDR_OFFSET 0x580 +#define SRC_RBUF_6_BASEADDR_OFFSET 0x598 + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_ENDADDR_REG_BASE */ +#define SRC_RBUF_0_ENDADDR_OFFSET 0x50c +#define SRC_RBUF_1_ENDADDR_OFFSET 0x524 +#define SRC_RBUF_2_ENDADDR_OFFSET 0x53c +#define SRC_RBUF_3_ENDADDR_OFFSET 0x554 +#define SRC_RBUF_4_ENDADDR_OFFSET 0x56c +#define SRC_RBUF_5_ENDADDR_OFFSET 0x584 +#define SRC_RBUF_6_ENDADDR_OFFSET 0x59c + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_FREE_MARK_REG_BASE */ +#define SRC_RBUF_0_FREE_MARK_OFFSET 0x510 +#define SRC_RBUF_1_FREE_MARK_OFFSET 0x528 +#define SRC_RBUF_2_FREE_MARK_OFFSET 0x540 +#define SRC_RBUF_3_FREE_MARK_OFFSET 0x558 +#define SRC_RBUF_4_FREE_MARK_OFFSET 0x570 +#define
[PATCH 2/2] ASoC: add core audio driver for Broadcom Cygnus SOC.
From: Lori Hikichi lhiki...@broadcom.com The audio block has 4 serial ports. 3 ports are configurable as either I2S or TDM. The 4th port is for SPDIF transmit. This audio block is found on the bcm958305, bcm958300, and bcm911360. Reviewed-by: Jonathan Richardson jonat...@broadcom.com Signed-off-by: Lori Hikichi lhiki...@broadcom.com Signed-off-by: Scott Branden sbran...@broadcom.com --- 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(-) create mode 100644 sound/soc/bcm/cygnus-pcm.c create mode 100644 sound/soc/bcm/cygnus-pcm.h create mode 100644 sound/soc/bcm/cygnus-ssp.c create mode 100644 sound/soc/bcm/cygnus-ssp.h diff --git a/sound/soc/bcm/Kconfig b/sound/soc/bcm/Kconfig index 6a834e1..2c5ff37 100644 --- a/sound/soc/bcm/Kconfig +++ b/sound/soc/bcm/Kconfig @@ -7,3 +7,14 @@ config SND_BCM2835_SOC_I2S Say Y or M if you want to add support for codecs attached to the BCM2835 I2S interface. You will also need to select the audio interfaces to support below. + +config SND_SOC_CYGNUS + tristate SoC platform audio for Broadcom Cygnus chips + depends on ARCH_BCM_CYGNUS || COMPILE_TEST + default ARCH_BCM_CYGNUS + help + Say Y if you want to add support for ASoC audio on Broadcom + Cygnus chips (bcm958300, bcm958305, bcm911360) + + If you don't know what to do here, say N. + diff --git a/sound/soc/bcm/Makefile b/sound/soc/bcm/Makefile index bc816b7..5c39790 100644 --- a/sound/soc/bcm/Makefile +++ b/sound/soc/bcm/Makefile @@ -1,5 +1,8 @@ # BCM2835 Platform Support snd-soc-bcm2835-i2s-objs := bcm2835-i2s.o -obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o +# CYGNUS Platform Support +snd-soc-cygnus-objs := cygnus-pcm.o cygnus-ssp.o +obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o +obj-$(CONFIG_SND_SOC_CYGNUS) += snd-soc-cygnus.o diff --git a/sound/soc/bcm/cygnus-pcm.c b/sound/soc/bcm/cygnus-pcm.c new file mode 100644 index 000..3a4106b --- /dev/null +++ b/sound/soc/bcm/cygnus-pcm.c @@ -0,0 +1,918 @@ +/* + * Copyright (C) 2014-2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed as is WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include linux/init.h +#include linux/module.h +#include linux/dma-mapping.h +#include linux/io.h +#include linux/slab.h +#include linux/debugfs.h +#include sound/core.h +#include sound/soc.h +#include sound/soc-dai.h +#include sound/pcm.h +#include sound/pcm_params.h +#include linux/io.h +#include linux/timer.h + +#include cygnus-ssp.h +#include cygnus-pcm.h + +/* Register offset needed for ASoC PCM module */ + +#define INTH_R5F_STATUS_OFFSET 0x040 +#define INTH_R5F_CLEAR_OFFSET 0x048 +#define INTH_R5F_MASK_SET_OFFSET 0x050 +#define INTH_R5F_MASK_CLEAR_OFFSET 0x054 + +#define BF_REARM_FREE_MARK_OFFSET 0x344 +#define BF_REARM_FULL_MARK_OFFSET 0x348 + +/* Ring Buffer Ctrl Regs --- Start */ +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_RDADDR_REG_BASE */ +#define SRC_RBUF_0_RDADDR_OFFSET 0x500 +#define SRC_RBUF_1_RDADDR_OFFSET 0x518 +#define SRC_RBUF_2_RDADDR_OFFSET 0x530 +#define SRC_RBUF_3_RDADDR_OFFSET 0x548 +#define SRC_RBUF_4_RDADDR_OFFSET 0x560 +#define SRC_RBUF_5_RDADDR_OFFSET 0x578 +#define SRC_RBUF_6_RDADDR_OFFSET 0x590 + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_WRADDR_REG_BASE */ +#define SRC_RBUF_0_WRADDR_OFFSET 0x504 +#define SRC_RBUF_1_WRADDR_OFFSET 0x51c +#define SRC_RBUF_2_WRADDR_OFFSET 0x534 +#define SRC_RBUF_3_WRADDR_OFFSET 0x54c +#define SRC_RBUF_4_WRADDR_OFFSET 0x564 +#define SRC_RBUF_5_WRADDR_OFFSET 0x57c +#define SRC_RBUF_6_WRADDR_OFFSET 0x594 + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_BASEADDR_REG_BASE */ +#define SRC_RBUF_0_BASEADDR_OFFSET 0x508 +#define SRC_RBUF_1_BASEADDR_OFFSET 0x520 +#define SRC_RBUF_2_BASEADDR_OFFSET 0x538 +#define SRC_RBUF_3_BASEADDR_OFFSET 0x550 +#define SRC_RBUF_4_BASEADDR_OFFSET 0x568 +#define SRC_RBUF_5_BASEADDR_OFFSET 0x580 +#define SRC_RBUF_6_BASEADDR_OFFSET 0x598 + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_ENDADDR_REG_BASE */ +#define SRC_RBUF_0_ENDADDR_OFFSET 0x50c +#define SRC_RBUF_1_ENDADDR_OFFSET 0x524 +#define SRC_RBUF_2_ENDADDR_OFFSET 0x53c +#define SRC_RBUF_3_ENDADDR_OFFSET 0x554 +#define SRC_RBUF_4_ENDADDR_OFFSET 0x56c +#define SRC_RBUF_5_ENDADDR_OFFSET 0x584 +#define SRC_RBUF_6_ENDADDR_OFFSET 0x59c + +/* AUD_FMM_BF_CTRL_SOURCECH_RINGBUF_X_FREE_MARK_REG_BASE
