Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension
On Wed, May 25, 2016 at 11:49:12AM +0100, Jose Abreu wrote:
> Ok, will do that. I noticed the last I2S patch that you merged
> ("ASoC: dwc: Add helper functions to disable/enable irqs") is not
> in for-next yet. Should I base my work on 'topic/dwc' branch?
We are in the merge window. No new non-bugfix patches will be merged
until the merge window ends, most likely sometime this weekend.
signature.asc
Description: PGP signature
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension
Hi Mark,
On 25-05-2016 11:18, Mark Brown wrote:
> On Wed, May 25, 2016 at 11:11:47AM +0100, Jose Abreu wrote:
>
>> I think I will take the second option. Something like this:
>> "
>> ret = snd_dmaengine_pcm_register(...)
>> if (ret == -EPROBE_DEFER)
>> return ret;
>> else
>> pio_register(...);
>> "?
> Sure. You should print a diagnostic if you fail to get the DMA, for any
> real system it's going to be a bug.
Ok, will do that. I noticed the last I2S patch that you merged
("ASoC: dwc: Add helper functions to disable/enable irqs") is not
in for-next yet. Should I base my work on 'topic/dwc' branch?
Best regards,
Jose Miguel Abreu
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension
Hi Mark, On 24-05-2016 18:51, Mark Brown wrote: > On Tue, May 24, 2016 at 06:07:14PM +0100, Jose Abreu wrote: >> On 24-05-2016 17:41, Mark Brown wrote: > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to. > if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) i2s_write_reg(dev->i2s_base, ITER, 1); >>> That seems wrong, or at least something that should be separate? >>> Previously we needed interrupts for DMA operation but now we enable >>> interrupts only if we don't use DMA. It feels like we want to make the >>> change for DMA separately if only to make it clear for bisection, are we >>> 100% sure that masking the interrupt won't also mask the DMA request >>> signals? >> Indeed I thought about this and the interrupts must also be enabled when in >> DMA >> mode. Although there is no interrupt handler in the original driver (without >> this patches) in some setups the interrupt line may be connected to the DMA >> controller. I will drop this change and always enable interrupts. Please note >> that I don't have a setup with DMA support so I can only test using the PIO >> mode. > Presumably you can talk to your hardware colleagues and get them to make > you a FPGA with a DMA IP available? Its already in the todo list. > >>> This also seems wrong. We're forcing PIO if an interrupt is provided >>> rather than based on DMA being configured which means that if the >>> interrupt is wired up and happens to be described in DT we'll get worse >> How should I then determine which mode to use? >> - Check if DMA parameters are declared in DT, or >> - Check if snd_dmaengine_pcm_register() fails, or >> - Assume PIO mode will be used when compiling with PIO PCM, or >> - Something else ? > You could either unconditionally register the PIO driver and only > actually start using it if the driver is instantiated or you could check > to see if the registration function works (handling deferred probe - if > the DMA driver just didn't load yet you should wait for it). I think I will take the second option. Something like this: " ret = snd_dmaengine_pcm_register(...) if (ret == -EPROBE_DEFER) return ret; else pio_register(...); "? Best regards, Jose Miguel Abreu ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension
On Tue, May 24, 2016 at 06:07:14PM +0100, Jose Abreu wrote: > On 24-05-2016 17:41, Mark Brown wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > >>if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > >>i2s_write_reg(dev->i2s_base, ITER, 1); > > That seems wrong, or at least something that should be separate? > > Previously we needed interrupts for DMA operation but now we enable > > interrupts only if we don't use DMA. It feels like we want to make the > > change for DMA separately if only to make it clear for bisection, are we > > 100% sure that masking the interrupt won't also mask the DMA request > > signals? > Indeed I thought about this and the interrupts must also be enabled when in > DMA > mode. Although there is no interrupt handler in the original driver (without > this patches) in some setups the interrupt line may be connected to the DMA > controller. I will drop this change and always enable interrupts. Please note > that I don't have a setup with DMA support so I can only test using the PIO > mode. Presumably you can talk to your hardware colleagues and get them to make you a FPGA with a DMA IP available? > > This also seems wrong. We're forcing PIO if an interrupt is provided > > rather than based on DMA being configured which means that if the > > interrupt is wired up and happens to be described in DT we'll get worse > How should I then determine which mode to use? > - Check if DMA parameters are declared in DT, or > - Check if snd_dmaengine_pcm_register() fails, or > - Assume PIO mode will be used when compiling with PIO PCM, or > - Something else ? You could either unconditionally register the PIO driver and only actually start using it if the driver is instantiated or you could check to see if the registration function works (handling deferred probe - if the DMA driver just didn't load yet you should wait for it). signature.asc Description: PGP signature ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension
Hi Mark,
Thanks for your comments.
On 24-05-2016 17:41, Mark Brown wrote:
> On Mon, May 23, 2016 at 11:02:24AM +0100, Jose Abreu wrote:
>
>> +config SND_DESIGNWARE_PCM
>> +bool "PCM PIO extension for I2S driver"
> Why can't this be built as a module?
I can change but my intention was to make this PCM a kind of extension to the
driver instead of adding a new module to the system.
>
>> +
>> +return irq_valid ? IRQ_HANDLED : IRQ_NONE;
> Please write a normal if statement, the ternery operator doesn't help
> legibility.
Ok.
>
>> static void i2s_start(struct dw_i2s_dev *dev,
>>struct snd_pcm_substream *substream)
>> {
>> struct i2s_clk_config_data *config = >config;
>>
>> i2s_write_reg(dev->i2s_base, IER, 1);
>> -i2s_enable_irqs(dev, substream->stream, config->chan_nr);
>> +
>> +if (dev->use_pio)
>> +i2s_enable_irqs(dev, substream->stream, config->chan_nr);
>>
>> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> i2s_write_reg(dev->i2s_base, ITER, 1);
> That seems wrong, or at least something that should be separate?
> Previously we needed interrupts for DMA operation but now we enable
> interrupts only if we don't use DMA. It feels like we want to make the
> change for DMA separately if only to make it clear for bisection, are we
> 100% sure that masking the interrupt won't also mask the DMA request
> signals?
Indeed I thought about this and the interrupts must also be enabled when in DMA
mode. Although there is no interrupt handler in the original driver (without
this patches) in some setups the interrupt line may be connected to the DMA
controller. I will drop this change and always enable interrupts. Please note
that I don't have a setup with DMA support so I can only test using the PIO
mode.
>> +irq = platform_get_irq(pdev, 0);
>> +if (irq >= 0) {
>> +dev_dbg(>dev, "using PIO mode\n");
>> +dev->use_pio = true;
>> +
>> +ret = devm_request_irq(>dev, irq, i2s_irq_handler, 0,
>> +pdev->name, dev);
>> +if (ret < 0) {
>> +dev_err(>dev, "failed to request irq\n");
>> +return ret;
>> +}
>> +}
> This also seems wrong. We're forcing PIO if an interrupt is provided
> rather than based on DMA being configured which means that if the
> interrupt is wired up and happens to be described in DT we'll get worse
> performance. People should be able to just describe the system without
> worrying about this, and we might find some other use for the interrupts
> in future. Indeed right now it would probably be reasonable to use the
> error interrupts all the time if they're available.
How should I then determine which mode to use?
- Check if DMA parameters are declared in DT, or
- Check if snd_dmaengine_pcm_register() fails, or
- Assume PIO mode will be used when compiling with PIO PCM, or
- Something else ?
Best regards,
Jose Miguel Abreu
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
