by Code Kipper

[permalink] [raw]

Subject: Re: [PATCH 04/14] ASoC: Add sun8i analog codec driver

On 4 October 2016 at 11:46, Mylène Josserand
<[email protected]> wrote:
> Add the analog part of the sun8i (A33) codec driver. This driver
> implements all the analog part of the codec using PRCM registers.
>
> The read/write regmap functions must be handled in a custom way as
> the PRCM register behaves as "mailbox" register.
>
> Signed-off-by: Mylène Josserand <[email protected]>
> ---
> sound/soc/sunxi/Kconfig | 7 +
> sound/soc/sunxi/Makefile | 1 +
> sound/soc/sunxi/sun8i-codec-analog.c | 304 +++++++++++++++++++++++++++++++++++
> 3 files changed, 312 insertions(+)
> create mode 100644 sound/soc/sunxi/sun8i-codec-analog.c
>
> diff --git a/sound/soc/sunxi/Kconfig b/sound/soc/sunxi/Kconfig
> index dd23682..7aee95a 100644
> --- a/sound/soc/sunxi/Kconfig
> +++ b/sound/soc/sunxi/Kconfig
> @@ -26,4 +26,11 @@ config SND_SUN4I_SPDIF
> help
> Say Y or M to add support for the S/PDIF audio block in the Allwinner
> A10 and affiliated SoCs.
> +
> +config SND_SUN8I_CODEC_ANALOG
> + tristate "Allwinner SUN8I analog codec"
> + select REGMAP_MMIO
> + help
> + Say Y or M if you want to add sun8i analog audiocodec support
> +
> endmenu
> diff --git a/sound/soc/sunxi/Makefile b/sound/soc/sunxi/Makefile
> index 604c7b84..241c0df 100644
> --- a/sound/soc/sunxi/Makefile
> +++ b/sound/soc/sunxi/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_SND_SUN4I_CODEC) += sun4i-codec.o
> obj-$(CONFIG_SND_SUN4I_I2S) += sun4i-i2s.o
> obj-$(CONFIG_SND_SUN4I_SPDIF) += sun4i-spdif.o
> +obj-$(CONFIG_SND_SUN8I_CODEC_ANALOG) += sun8i-codec-analog.o
> diff --git a/sound/soc/sunxi/sun8i-codec-analog.c b/sound/soc/sunxi/sun8i-codec-analog.c
> new file mode 100644
> index 0000000..be3d540
> --- /dev/null
> +++ b/sound/soc/sunxi/sun8i-codec-analog.c
> @@ -0,0 +1,304 @@
> +/*
> + * This driver supports the analog controls for the internal codec
> + * found in Allwinner's A31s, A33 and A23 SoCs.
> + *
> + * Copyright 2016 Chen-Yu Tsai <[email protected]>
> + * Copyright 2016 Mylène Josserand <[email protected]>
> + *
> + * 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; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <sound/soc.h>
> +#include <sound/soc-dapm.h>
> +#include <sound/tlv.h>
> +
> +/* Codec analog control register offsets and bit fields */
> +#define SUN8I_ADDA_HP_VOLC 0x00
> +#define SUN8I_ADDA_HP_VOLC_PA_CLK_GATE 7
> +#define SUN8I_ADDA_HP_VOLC_HP_VOL 0
> +#define SUN8I_ADDA_LOMIXSC 0x01
> +#define SUN8I_ADDA_LOMIXSC_MIC1 6
> +#define SUN8I_ADDA_LOMIXSC_MIC2 5
> +#define SUN8I_ADDA_LOMIXSC_PHONE 4
> +#define SUN8I_ADDA_LOMIXSC_PHONEN 3
> +#define SUN8I_ADDA_LOMIXSC_LINEINL 2
> +#define SUN8I_ADDA_LOMIXSC_DACL 1
> +#define SUN8I_ADDA_LOMIXSC_DACR 0
> +#define SUN8I_ADDA_ROMIXSC 0x02
> +#define SUN8I_ADDA_ROMIXSC_MIC1 6
> +#define SUN8I_ADDA_ROMIXSC_MIC2 5
> +#define SUN8I_ADDA_ROMIXSC_PHONE 4
> +#define SUN8I_ADDA_ROMIXSC_PHONEP 3
> +#define SUN8I_ADDA_ROMIXSC_LINEINR 2
> +#define SUN8I_ADDA_ROMIXSC_DACR 1
> +#define SUN8I_ADDA_ROMIXSC_DACL 0
> +#define SUN8I_ADDA_DAC_PA_SRC 0x03
> +#define SUN8I_ADDA_DAC_PA_SRC_DACAREN 7
> +#define SUN8I_ADDA_DAC_PA_SRC_DACALEN 6
> +#define SUN8I_ADDA_DAC_PA_SRC_RMIXEN 5
> +#define SUN8I_ADDA_DAC_PA_SRC_LMIXEN 4
> +#define SUN8I_ADDA_DAC_PA_SRC_RHPPAMUTE 3
> +#define SUN8I_ADDA_DAC_PA_SRC_LHPPAMUTE 2
> +#define SUN8I_ADDA_DAC_PA_SRC_RHPIS 1
> +#define SUN8I_ADDA_DAC_PA_SRC_LHPIS 0
> +#define SUN8I_ADDA_PHONEIN_GCTRL 0x04
> +#define SUN8I_ADDA_PHONEIN_GCTRL_PHONEPG 4
> +#define SUN8I_ADDA_PHONEIN_GCTRL_PHONENG 0
> +#define SUN8I_ADDA_LINEIN_GCTRL 0x05
> +#define SUN8I_ADDA_LINEIN_GCTRL_LINEING 4
> +#define SUN8I_ADDA_LINEIN_GCTRL_PHONEG 0
> +#define SUN8I_ADDA_MICIN_GCTRL 0x06
> +#define SUN8I_ADDA_MICIN_GCTRL_MIC1G 4
> +#define SUN8I_ADDA_MICIN_GCTRL_MIC2G 0
> +#define SUN8I_ADDA_PAEN_HP_CTRL 0x07
> +#define SUN8I_ADDA_PAEN_HP_CTRL_HPPAEN 7
> +#define SUN8I_ADDA_PAEN_HP_CTRL_HPCOM_FC 5
> +#define SUN8I_ADDA_PAEN_HP_CTRL_COMPTEN 4
> +#define SUN8I_ADDA_PAEN_HP_CTRL_PA_ANTI_POP_CTRL 2
> +#define SUN8I_ADDA_PAEN_HP_CTRL_LTRNMUTE 1
> +#define SUN8I_ADDA_PAEN_HP_CTRL_RTLNMUTE 0
> +#define SUN8I_ADDA_PHONEOUT_CTRL 0x08
> +#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTG 5
> +#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTEN 4
> +#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTS3 3
> +#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTS2 2
> +#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTS1 1
> +#define SUN8I_ADDA_PHONEOUT_CTRL_PHONEOUTS0 0
> +#define SUN8I_ADDA_PHONE_GAIN_CTRL 0x09
> +#define SUN8I_ADDA_PHONE_GAIN_CTRL_PHONEPREG 0
> +#define SUN8I_ADDA_MIC2G_CTRL 0x0a
> +#define SUN8I_ADDA_MIC2G_CTRL_MIC2AMPEN 7
> +#define SUN8I_ADDA_MIC2G_CTRL_MIC2BOOST 4
> +#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL 0x0b
> +#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_HMICBIASEN 7
> +#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MMICBIASEN 6
> +#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_HMICBIAS_MODE 5
> +#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MIC1AMPEN 3
> +#define SUN8I_ADDA_MIC1G_MICBIAS_CTRL_MIC1BOOST 0
> +#define SUN8I_ADDA_PA_ANTI_POP_CTRL 0x0e
> +#define SUN8I_ADDA_ADC_AP_EN 0x0f
> +
> +/* Analog control register access bits */
> +#define ADDA_PR 0x0 /* PRCM base + 0x1c0 */
> +#define ADDA_PR_RESET BIT(28)
> +#define ADDA_PR_WRITE BIT(24)
> +#define ADDA_PR_ADDR_SHIFT 16
> +#define ADDA_PR_ADDR_MASK GENMASK(4, 0)
> +#define ADDA_PR_DATA_IN_SHIFT 8
> +#define ADDA_PR_DATA_IN_MASK GENMASK(7, 0)
> +#define ADDA_PR_DATA_OUT_SHIFT 0
> +#define ADDA_PR_DATA_OUT_MASK GENMASK(7, 0)
> +
> +/* regmap access bits */
> +static int adda_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + void __iomem *base = context;
> + u32 tmp;
> +
> + tmp = readl(base);
> +
> + /* De-assert reset */
> + writel(tmp | ADDA_PR_RESET, base);
> +
> + tmp &= ~(ADDA_PR_ADDR_MASK << ADDA_PR_ADDR_SHIFT);
> + tmp |= reg << ADDA_PR_ADDR_SHIFT;
> + writel(tmp, base);
> +
> + *val = readl(base) & ADDA_PR_DATA_OUT_MASK;
> +
> + return 0;
> +}
> +
> +static int adda_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> + void __iomem *base = context;
> + u32 tmp;
> +
> + tmp = readl(base);
> + /* De-assert reset */
> + writel(tmp | ADDA_PR_RESET, base);
> +
> + /* Write the address */
> + tmp &= ~(ADDA_PR_ADDR_MASK << ADDA_PR_ADDR_SHIFT);
> + tmp |= reg << ADDA_PR_ADDR_SHIFT;
> + writel(tmp, base);
> +
> + /* Write the value */
> + tmp &= ~(ADDA_PR_DATA_IN_MASK << ADDA_PR_DATA_IN_SHIFT);
> + tmp |= val << ADDA_PR_DATA_IN_SHIFT;
> + writel(tmp, base);
> +
> + /* Indicate that the previous value must be written */
> + writel(readl(base) | ADDA_PR_WRITE, base);
> +
> + /* Reset the write bit */
> + writel(readl(base) & ~(ADDA_PR_WRITE), base);
> +
> + return 0;
> +}
> +
> +struct regmap_config adda_pr_regmap_cfg = {
> + .name = "adda-pr",
> + .reg_bits = 5,
> + .reg_stride = 1,
> + .val_bits = 8,
> + .reg_read = adda_reg_read,
> + .reg_write = adda_reg_write,
> + .fast_io = true,
> + .max_register = 24,
> +};
> +
> +static DECLARE_TLV_DB_SCALE(sun8i_codec_headphone_volume_scale, -6300, 100, 1);
> +
> +static const struct snd_kcontrol_new sun8i_analog_widgets[] = {
> + SOC_SINGLE_TLV("Headphone Volume", SUN8I_ADDA_HP_VOLC,
> + SUN8I_ADDA_HP_VOLC_HP_VOL, 0x3F, 0,
> + sun8i_codec_headphone_volume_scale),
> +
> + /* Playback Switch */
> + SOC_DOUBLE("DAC Playback Switch", SUN8I_ADDA_DAC_PA_SRC,
> + SUN8I_ADDA_DAC_PA_SRC_DACALEN, SUN8I_ADDA_DAC_PA_SRC_DACAREN,
> + 1, 0),
> +
> + SOC_DOUBLE("Headphone Playback Switch", SUN8I_ADDA_DAC_PA_SRC,
> + SUN8I_ADDA_DAC_PA_SRC_LHPPAMUTE,
> + SUN8I_ADDA_DAC_PA_SRC_RHPPAMUTE, 1, 0),
> +};
> +
> +/* headphone controls */
> +static const char * const sun8i_codec_hp_src_enum_text[] = {
> + "DAC", "Mixer",
> +};
> +
> +static SOC_ENUM_DOUBLE_DECL(sun8i_codec_hp_src_enum,
> + SUN8I_ADDA_DAC_PA_SRC,
> + SUN8I_ADDA_DAC_PA_SRC_LHPIS,
> + SUN8I_ADDA_DAC_PA_SRC_RHPIS,
> + sun8i_codec_hp_src_enum_text);
> +
> +static const struct snd_kcontrol_new sun8i_codec_hp_src[] = {
> + SOC_DAPM_ENUM("Headphone Source Playback Route",
> + sun8i_codec_hp_src_enum),
> +};
> +
> +static const struct snd_kcontrol_new sun8i_codec_mixer_controls[] = {
> + SOC_DAPM_SINGLE("DAC Left Playback Switch",
> + SUN8I_ADDA_LOMIXSC,
> + SUN8I_ADDA_LOMIXSC_DACL, 1, 0),
> + SOC_DAPM_SINGLE("DAC Right Playback Switch",
> + SUN8I_ADDA_ROMIXSC,
> + SUN8I_ADDA_ROMIXSC_DACR, 1, 0),
> + SOC_DAPM_SINGLE("DAC Reversed Left Playback Switch",
> + SUN8I_ADDA_LOMIXSC,
> + SUN8I_ADDA_LOMIXSC_DACR, 1, 0),
> + SOC_DAPM_SINGLE("DAC Reversed Right Playback Switch",
> + SUN8I_ADDA_ROMIXSC,
> + SUN8I_ADDA_ROMIXSC_DACL, 1, 0),
> +};
> +
> +static const struct snd_soc_dapm_widget sun8i_codec_analog_dapm_widgets[] = {
> + /* Mixers */
> + SOC_MIXER_ARRAY("Left Mixer", SUN8I_ADDA_DAC_PA_SRC,
> + SUN8I_ADDA_DAC_PA_SRC_LMIXEN, 0,
> + sun8i_codec_mixer_controls),
> + SOC_MIXER_ARRAY("Right Mixer", SUN8I_ADDA_DAC_PA_SRC,
> + SUN8I_ADDA_DAC_PA_SRC_RMIXEN, 0,
> + sun8i_codec_mixer_controls),
> +
> + /* Headphone output path */
> + SND_SOC_DAPM_MUX("Headphone Source Playback Route",
> + SND_SOC_NOPM, 0, 0, sun8i_codec_hp_src),
> + SND_SOC_DAPM_OUT_DRV("Headphone Amp", SUN8I_ADDA_PAEN_HP_CTRL,
> + SUN8I_ADDA_PAEN_HP_CTRL_HPPAEN, 0, NULL, 0),
> +
> + /* Headphone outputs */
> + SND_SOC_DAPM_OUTPUT("HP"),
> +
> +};
> +
> +static const struct snd_soc_dapm_route sun8i_codec_analog_dapm_routes[] = {
> + /* Left Mixer Routes */
> + { "Left Mixer", "DAC Playback Switch", "Left DAC" },
> + { "Left Mixer", "DAC Reversed Left Playback Switch", "Right DAC" },
> +
> + /* Right Mixer Routes */
> + { "Right Mixer", "DAC Playback Switch", "Right DAC" },
> + { "Right Mixer", "DAC Reversed Right Playback Switch", "Left DAC" },
> +
> + /* Headphone Routes */
> + { "Headphone Source Playback Route", "DAC", "Left DAC" },
> + { "Headphone Source Playback Route", "DAC", "Right DAC" },
> + { "Headphone Source Playback Route", "Mixer", "Left Mixer" },
> + { "Headphone Source Playback Route", "Mixer", "Right Mixer" },
> + { "Headphone Amp", NULL, "Headphone Source Playback Route" },
> + { "HP", NULL, "Headphone Amp" },
> +};
> +
> +static const struct snd_soc_component_driver sun8i_codec_analog_cmpnt_drv = {
> + .name = "sun8i-codec-analog",
> + .controls = sun8i_analog_widgets,
> + .num_controls = ARRAY_SIZE(sun8i_analog_widgets),
> + .dapm_widgets = sun8i_codec_analog_dapm_widgets,
> + .num_dapm_widgets = ARRAY_SIZE(sun8i_codec_analog_dapm_widgets),
> + .dapm_routes = sun8i_codec_analog_dapm_routes,
> + .num_dapm_routes = ARRAY_SIZE(sun8i_codec_analog_dapm_routes),
> +};
> +
> +static const struct of_device_id sun8i_codec_analog_of_match[] = {
> + { .compatible = "allwinner,sun8i-codec-analog", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_codec_analog_of_match);
> +
> +static int sun8i_codec_analog_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + struct regmap *regmap;
> + void __iomem *base;
> +
> + /* Get PRCM resources and registers */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev, "Failed to map PRCM registers\n");
> + return PTR_ERR(base);
> + }
> +
> + regmap = devm_regmap_init(&pdev->dev, NULL, base, &adda_pr_regmap_cfg);
> + if (IS_ERR(regmap)) {
> + dev_err(&pdev->dev, "Regmap initialisation failed\n");
> + return PTR_ERR(regmap);
> + }
> +
> + return devm_snd_soc_register_component(&pdev->dev,
> + &sun8i_codec_analog_cmpnt_drv,
> + NULL, 0);
> +}
> +
> +static struct platform_driver sun8i_codec_analog_driver = {
> + .driver = {
> + .name = "sun8i-codec-analog",
> + .of_match_table = sun8i_codec_analog_of_match,
> + },
> + .probe = sun8i_codec_analog_probe,
> +};
> +module_platform_driver(sun8i_codec_analog_driver);
> +
> +MODULE_DESCRIPTION("Allwinner A31s/A33/A23 codec analog controls driver");
Does the A31s have the prcm for the codec?, as I was under the
impression that it was the same as the A31 and I can't seem to find a
datasheet for that SoC. You could also add H3 and A64 here.
CK
> +MODULE_AUTHOR("Chen-Yu Tsai <[email protected]>");
> +MODULE_AUTHOR("Mylène Josserand <[email protected]>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:sun8i-codec-analog");
> --
> 2.9.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2016-10-04 10:33:48

On Tue, Oct 04, 2016 at 02:40:08PM +0200, Thomas Petazzoni wrote:

> > +/* CODEC_OFFSET represents the offset of the codec registers
> > + * and not all the DAI registers
> > + */

> This is not the proper comment style I believe for audio code, it
> should be:

> /*
> * ...
> */

I don't care, IIRC that's something from CodingStyle which checkpatch
moans about.

> > + /* pcm operations */
> > + .ops = &sun8i_codec_dai_ops,
> > +};
> > +EXPORT_SYMBOL(sun8i_codec_dai);

> This EXPORT_SYMBOL looks wrong. First because it doesn't seem to be
> used outside of this module. And second because using EXPORT_SYMBOL on
> a function defined as static doesn't make much sense, as the "static"
> qualifier limits the visibility of the symbol to the current
> compilation unit.

Also all the ASoC code is _GPL so a non-GPL export is an issue.

> > + .component_driver = {
> > + .dapm_widgets = sun8i_codec_dapm_widgets,
> > + .num_dapm_widgets = ARRAY_SIZE(sun8i_codec_dapm_widgets),
> > + .dapm_routes = sun8i_codec_dapm_routes,
> > + .num_dapm_routes = ARRAY_SIZE(sun8i_codec_dapm_routes),

> I'm probably missing something, but in the sun4i-codec.c driver, those
> fields are initialized directly in the snd_soc_codec_driver structure,
> not in the .component_driver sub-structure.

We're in the process of pushing everything out to component level, this
update should be made in the old code if it's not happened already.

> > + if (clk_prepare_enable(scodec->clk_module))
> > + pr_err("err:open failed;\n");

> Grr, pr_err, not good. Plus you want to return with an error from the
> probe() function.

Also when printing an error message use dev_err().

Attachments:

(No filename) (1.62 kB)
signature.asc (455.00 B)
Download all attachments

2016-10-04 13:16:09

by Thomas Petazzoni

[permalink] [raw]

Subject: Re: [PATCH 06/14] ASoC: Add sun8i digital audio codec

Hello,

On Tue, 4 Oct 2016 15:07:27 +0200, Mark Brown wrote:

> > /*
> > * ...
> > */
>
> I don't care, IIRC that's something from CodingStyle which checkpatch
> moans about.

Correct. The

/* ..
* ..
*/

style is mandatory for net/ and crypto code, but not in the rest of the
kernel.

> > I'm probably missing something, but in the sun4i-codec.c driver, those
> > fields are initialized directly in the snd_soc_codec_driver structure,
> > not in the .component_driver sub-structure.
>
> We're in the process of pushing everything out to component level, this
> update should be made in the old code if it's not happened already.

OK.

> > > + if (clk_prepare_enable(scodec->clk_module))
> > > + pr_err("err:open failed;\n");
>
> > Grr, pr_err, not good. Plus you want to return with an error from the
> > probe() function.
>
> Also when printing an error message use dev_err().

That's why I said "Grr, pr_err, not good" :)

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

2016-10-04 13:47:01

by Jean-Francois Moine

[permalink] [raw]

Subject: Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4

On Tue, 4 Oct 2016 14:12:21 +0200
Thomas Petazzoni <[email protected]> wrote:

> > > Add the case of a burst of 4 which is handled by the SoC.
> > >
> > > Signed-off-by: Myl?ne Josserand <[email protected]>
> > > ---
> > > drivers/dma/sun6i-dma.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 8346199..0485204 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > > switch (maxburst) {
> > > case 1:
> > > return 0;
> > > + case 4:
> > > + return 1;
> > > case 8:
> > > return 2;
> > > default:
> > > --
> > > 2.9.3
> >
> > This patch has already been rejected by Maxime in the threads
> > http://www.spinics.net/lists/dmaengine/msg08610.html
> > and
> > http://www.spinics.net/lists/dmaengine/msg08719.html
> >
> > I hope you will find the way he wants for this maxburst to be added.
>
> I was about to reply to Mylene's e-mail, suggesting that she should add
> a comment in the code (and maybe in the commit log) to explain why this
> addition is needed, and also that even though the schematics say that
> value "1" (max burst size of 4 bytes) is reserved, it is in fact
> incorrect. The Allwinner BSP code is really using this value, and it's
> the value that makes audio work, so we believe the datasheet is simply
> incorrect.
>
> We already discussed it with Maxime, so I believe he should agree this
> time. But I would suggest to have such details explained in the commit
> log and in a comment in the code.

Strange. Looking at the datasheets of the A23, A31, A33, A83T and H3
(these are the SoCs using the DMA sun6i), only the H3 can have 4 as the
burst size (the doc is unclear for the A31).

Well, I was submitting for the H3, Myl?ne is submitting for the A33.
So, what about the A23, A31 and A83T?

--
Ken ar c'henta? | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

2016-10-04 15:31:36

by Vinod Koul

On Tue, Nov 01, 2016 at 10:55:13PM +0800, Chen-Yu Tsai wrote:

> >> * @src_maxburst: the maximum number of words (note: words, as in
> >> * units of the src_addr_width member, not bytes) that can be sent
> >> * in one burst to the device. Typically something like half the
> >> * FIFO depth on I/O peripherals so you don't overflow it. This
> >> * may or may not be applicable on memory sources.
> >> * @dst_maxburst: same as src_maxburst but for destination target
> >> * mutatis mutandis.
> >>
> >> The DMA engine driver should be free to select whatever burst size
> >> that doesn't exceed this. So for max_burst = 4, the driver can select
> >> burst = 4 for controllers that do support it, or burst = 1 for those
> >> that don't, and do more bursts.
> >
> > Nope, the client configures these parameters and dmaengine driver
> > validates and programs
>
> Shouldn't we just name it "burst_size" then if it's meant to be what
> the client specifically asks for?

Well if for some reason we program lesser than than max it would work
technically. But a larger burst wont work at all, so thats why maxburst is
significant.

> My understanding is that the client configures its own parameters,
> such as the trigger level for the DRQ, like raise DRQ when level < 1/4
> FIFO depth, request maxburst = 1/4 or 1/2 FIFO depth, so as not to
> overrun the FIFO. When the DRQ is raised, the DMA engine will do a
> burst, and after the burst the DRQ would be low again, so the DMA
> engine will wait. So the DMA engine driver should be free to
> program the actual burst size to something less than maxburst, shouldn't
> it?

Yup but not more that max..

> >> This also means we can increase max_burst for the audio codec, as
> >> the FIFO is 64 samples deep for stereo, or 128 samples for mono.
> >
> > Beware that higher bursts means chance of underrun of FIFO. This value
> > is selected with consideration of power and performance required. Lazy
> > allocation would be half of FIFO size..
>
> You mean underrun if its the source right? So the client setting maxburst
> should take the DRQ trigger level into account for this.

Yes

--
~Vinod