Re: [PATCH v5 00/17] ASoC: fsl_ssi: Clean up - program flow level

[Maciej S. Szmigiero]

On 17.01.2018 21:02, Nicolin Chen wrote:
> On Wed, Jan 17, 2018 at 08:38:48PM +0100, Maciej S. Szmigiero wrote:
> 
>> However, I have a small nitpick regarding a comment newly added in
>> this version of patch 16:
>> +/*
>> + * Do not set SSI dev as the parent of AC97 CODEC device since
>> + * it does not have a DT node. Otherwise ASoC core will assume
>> + * CODEC has the same DT node as the SSI, so it may return a
>> + * NULL pointer of CODEC when asked for SSI via the DT node
>>
>> The second part of the last sentence isn't really true, the ASoC core
>> will return a (valid, non-NULL) CODEC object pointer when asked for
>> the SSI one if we set the SSI as the parent device of a AC'97 CODEC
>> platform device.
>>
>> The NULL pointer dereference when starting a playback that I wrote
>> about in my previous message happens because in this situation the SSI
>> DAI probe callback won't ever get called and so won't setup DMA data
>> pointers (they will remain NULL).
> 
> Well, somehow the DMA data pointer of CODEC could be described
> as "a NULL pointer of CODEC" reluctantly...it confuses people
> though.
> 
>> And this in turn will cause the ASoC DMA code to dereference these
>> NULL pointers when starting a playback (the same will probably happen
>> also when starting a capture).
>>
>> Sorry if I wasn't 100% clear about these details in my previous
>> message describing this issue.
> 
> I would prefer to send an incremental patch later to update it,
> if there are no new comments against this version; Otherwise, I
> will update it in a next version once there is a need to send a
> v6 anyway.

IMHO it is such a tiny thing that it isn't worth respinning 17
patch series just for it, it can be easily improved later via
a separate patch.

> Thanks
> 

Thanks,
Maciej

Re: [PATCH v5 00/17] ASoC: fsl_ssi: Clean up - program flow level

[Maciej S. Szmigiero]

On 17.01.2018 07:51, Nicolin Chen wrote:
> [ Maciej, could you please send your Tested-by/Reviewed-by for AC97
>   once you confirm this series?
>   
>   And Caleb, this version does not need a test for non-AC97 cases.
> 
>   Thanks both! ]
> 
> ==Change log==
> v5
>  * Reworked the series by taking suggestions from Maciej for AC97
>   + Fixed SSI lockup issue by changing cleanup sequence in PATCH-13
>   + Moved fsl_ssi_hw_clean() after unregistering the CODEC device
> in PATCH-13
>   + Set NULL as the parent of CODEC platform device to fix a NULL
> pointer dereference bug in PATCH-16
>  * Updated comments of three variables/pointers in struct fsl_ssi
>to describe them more accurately in PATCH-16
> v4
>  * Reworked the series by taking suggestions from Maciej
>   + Added TXBIT0 bit back to play safe in PATCH-14
>   + Made bool synchronous exclusive with AC97 mode in PATCH-16
> v3
>  * Reworked the series by taking suggestions from Maciej
>   + Added PATCH-01 to make RX and TX more clearly defined
>   + Replaced "bool dir" with "int dir" in PATCH-04
>   + Replaced "!dir" with "int adir" in PATCH-05
>   + Put CBM_CFS behind the baudclk check to keep the same
> program flow in PATCH-14
>   + Removed all cpu_dai_drv changes in PATCH-15
> v2
>  * Reworked the series by taking suggestions from Maciej
>   + Added PATCH-01 to keep all ssi->i2s_net updated
>   + Replaced bool tx with bool dir in PATCH-03 and PATCH-06
>   + Moved all initial register configurations from dai probe() to
> platform probe() so as to let AC97 CODEC successfully probe.
>  * Added Tested-by from Caleb for TDM test cases.
> 
> ==Background==
> The fsl_ssi driver was designed for PPC originally and then it has
> been updated to support different modes for i.MX Series, including
> SDMA, I2S Master mode, AC97 and older i.MXs with FIQ, by different
> contributors for different use cases in different coding styles.
> 
> Additionally, in order to fix/work-around hardware bugs and design
> flaws, the driver made a lot of compromise so now its program flow
> looks very complicated and it's getting hard to maintain or update.
> 
> So I am going to clean up the driver on both coding style level and
> program flow level.
> 
> ==Introduction==
> This series of patches is the second set to clean up fsl_ssi driver
> in the program flow level. Any patch here may impact a fundamental
> test case like playback or record.
> 
> ==Verification==
> This series of patches require fully tested. I have done such tests
> on i.MX6SoloX with WM8962 using imx_v6_v7_defconfig as:
>  - Playback via I2S Master and Slave mode
>  - Record via I2S Master and Slave mode
>  - Simultaneous playback and record via I2S Master and Slave mode
>  - Background playback with foreground record (starting at different
>time) via I2S Master and Slave mode
>  - Background record with foreground playback (starting at different
>time) via I2S Master and Slave mode
>  * All tests above by hacking offline_config to true in imx51.
> 
> Caleb has tested v1-v4 with TDM lookback tests on i.MX6.
> 
> Example of uncovered tests: AC97, PowerPC and FIQ.

For the whole series:
Tested-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Reviewed-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>

However, I have a small nitpick regarding a comment newly added in
this version of patch 16:
+   /*
+* Do not set SSI dev as the parent of AC97 CODEC device since
+* it does not have a DT node. Otherwise ASoC core will assume
+* CODEC has the same DT node as the SSI, so it may return a
+* NULL pointer of CODEC when asked for SSI via the DT node

The second part of the last sentence isn't really true, the ASoC core
will return a (valid, non-NULL) CODEC object pointer when asked for
the SSI one if we set the SSI as the parent device of a AC'97 CODEC
platform device.

The NULL pointer dereference when starting a playback that I wrote
about in my previous message happens because in this situation the SSI
DAI probe callback won't ever get called and so won't setup DMA data
pointers (they will remain NULL).
And this in turn will cause the ASoC DMA code to dereference these
NULL pointers when starting a playback (the same will probably happen
also when starting a capture).

Sorry if I wasn't 100% clear about these details in my previous
message describing this issue.

Maciej

Re: [PATCH v4 00/17] ASoC: fsl_ssi: Clean up - program flow level

[Maciej S. Szmigiero]

On 17.01.2018 00:52, Nicolin Chen wrote:
> On Wed, Jan 17, 2018 at 12:38:09AM +0100, Maciej S. Szmigiero wrote:
> 
>>> Example of uncovered tests: AC97, PowerPC and FIQ.
>>
>> I've tested the whole series in the AC'97 mode on an i.MX6 UDOO board
>> and everything seems to work fine as long as few small changes are made
>> to patches 13 and 16.
> 
> Thanks for the review and testing. Really appreciate that. I'll
> take care of those issues in the next version.
Thanks for these cleanups Nicolin!

> And I believe those issues are confined to AC97. So Caleb should
> be able to skip his test if only these two places get updated.
> (Thanks for keeping testing on each version.)

Yes, these required changes only affect the AC'97 mode, there
shouldn't be any functionality change in the I2S mode.

Maciej

Re: [PATCH v4 16/17] ASoC: fsl_ssi: Move DT related code to a separate probe()

[Maciej S. Szmigiero]

On 16.01.2018 00:16, Nicolin Chen wrote:
> This patch cleans up probe() function by moving all Device Tree
> related code into a separate function. It allows the probe() to
> be Device Tree independent. This will be very useful for future
> integration of imx-ssi driver which has similar functionalities
> while exists only because it supports non-DT cases.
> 
> This patch also moves symmetric_channels of AC97 from the probe
> to the structure snd_soc_dai_driver for simplification.
> 
> Additionally, since PowerPC and AC97 use the same pdev pointer
> to register a platform device, this patch also unifies related
> code.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
> Changelog
> v4
>  * Made bool synchronous exclusive with AC97 mode in PATCH-16
> 
>  sound/soc/fsl/fsl_ssi.c | 209 
> ++--
>  1 file changed, 114 insertions(+), 95 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index dfeca43..07ec47d 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1529,50 +1581,17 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>   if (ret)
>   goto error_asoc_register;
>  
> - /* Bypass it if using newer DT bindings of ASoC machine drivers */
> - if (!of_get_property(np, "codec-handle", NULL))
> - goto done;
> -
> - /*
> -  * Backward compatible for older bindings by manually triggering the
> -  * machine driver's probe(). Use /compatible property, including the
> -  * address of CPU DAI driver structure, as the name of machine driver.
> -  */
> - sprop = of_get_property(of_find_node_by_path("/"), "compatible", NULL);
> - /* Sometimes the compatible name has a "fsl," prefix, so we strip it. */
> - p = strrchr(sprop, ',');
> - if (p)
> - sprop = p + 1;
> - snprintf(name, sizeof(name), "snd-soc-%s", sprop);
> - make_lowercase(name);
> -
> - ssi->pdev = platform_device_register_data(dev, name, 0, NULL, 0);
> - if (IS_ERR(ssi->pdev)) {
> - ret = PTR_ERR(ssi->pdev);
> - dev_err(dev, "failed to register platform: %d\n", ret);
> - goto error_sound_card;
> - }
> -
> -done:
>   /* Initially configures SSI registers */
>   fsl_ssi_hw_init(ssi);
>  
> - if (fsl_ssi_is_ac97(ssi)) {
> - u32 ssi_idx;
> -
> - ret = of_property_read_u32(np, "cell-index", _idx);
> - if (ret) {
> - dev_err(dev, "failed to get SSI index property\n");
> - goto error_sound_card;
> - }
> -
> - ssi->pdev = platform_device_register_data(NULL, "ac97-codec",
> -   ssi_idx, NULL, 0);
> - if (IS_ERR(ssi->pdev)) {
> - ret = PTR_ERR(ssi->pdev);
> - dev_err(dev,
> - "failed to register AC97 codec platform: %d\n",
> - ret);
> + /* Register a platform device for older bindings or AC97 */
> + if (ssi->card_name[0]) {
> + ssi->card_pdev = platform_device_register_data(dev,
   ^
Here we need to pass NULL as the parent in the AC'97 mode as the original
code did, because otherwise snd_soc_find_dai() will assume that the AC'97
CODEC platform device has the same DT node as the SSI (the CODEC isn't
present in the DT on its own) and so when asked for SSI by its DT node
will return the CODEC instead.

The end result is a NULL pointer dereference when starting a playback.

Maciej

Re: [PATCH v4 13/17] ASoC: fsl_ssi: Setup AC97 in fsl_ssi_hw_init()

[Maciej S. Szmigiero]

On 16.01.2018 00:16, Nicolin Chen wrote:
> AC97 configures most of registers earlier to start a communication
> with CODECs in order to successfully initialize CODEC. Currently,
> _fsl_ssi_set_dai_fmt() and fsl_ssi_setup_ac97() are called to get
> all SSI registers properly set.
> 
> Since now the driver has a fsl_ssi_hw_init() to handle all register
> initial settings, this patch moves those register settings of AC97
> to the fsl_ssi_hw_init() as well.
> 
> Meanwhile it applies _fsl_ssi_set_dai_fmt() call to AC97 only since
> other formats would be configured via normal set_dai_fmt() directly.
> 
> This patch also adds fsl_ssi_hw_clean() to cleanup control bits for
> AC97 in the platform remote() function.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
> Changelog
> v2
>  * Moved all to fsl_ssi_hw_init() in platform probe()
>  * Added fsl_ssi_hw_clean() instead of dai remove()
> 
>  sound/soc/fsl/fsl_ssi.c | 26 --
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 50648f5..ebb3eb9 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1255,10 +1252,28 @@ static int fsl_ssi_hw_init(struct fsl_ssi *ssi)
>   regmap_update_bits(ssi->regs, REG_SSI_SCR,
>  SSI_SCR_TCH_EN, SSI_SCR_TCH_EN);
>  
> + /* AC97 should start earlier to communicate with CODECs */
> + if (fsl_ssi_is_ac97(ssi)) {
> + _fsl_ssi_set_dai_fmt(ssi->dev, ssi, ssi->dai_fmt);
> + fsl_ssi_setup_ac97(ssi);
> + }
> +
>   return 0;
>  }
>  
>  /**
> + * Clear SSI registers
> + */
> +static void fsl_ssi_hw_clean(struct fsl_ssi *ssi)
> +{
> + /* Disable registers for AC97 */
> + if (fsl_ssi_is_ac97(ssi)) {
> + regmap_write(ssi->regs, REG_SSI_SCR, 0);
> + regmap_write(ssi->regs, REG_SSI_SACNT, 0);
> + regmap_write(ssi->regs, REG_SSI_SOR, 0);

These writes cause a hard SoC lockup when unloading the driver.

The following replacement order seems to be tolerated by the SSI:
regmap_update_bits(ssi->regs, REG_SSI_SCR, SSI_SCR_TE | SSI_SCR_RE, 0);
regmap_write(ssi->regs, REG_SSI_SACNT, 0);
regmap_write(ssi->regs, REG_SSI_SOR, 0);
regmap_update_bits(ssi->regs, REG_SSI_SCR, SSI_SCR_SSIEN, 0);

BTW: I've also tried disabling SSIEN before writing to SACNT and SOR
(both together with (TR | RE) and also as a separate step) but this
still caused a lockup.

> @@ -1587,6 +1599,8 @@ static int fsl_ssi_remove(struct platform_device *pdev)
>  {
>   struct fsl_ssi *ssi = dev_get_drvdata(>dev);
>  
> + fsl_ssi_hw_clean(ssi);
> +

We need to move this call...

>   fsl_ssi_debugfs_remove(>dbg_stats);
>  
>   if (ssi->pdev)
>   platform_device_unregister(ssi->card_pdev);

...here, after the AC'97 CODEC platform device is unregistered, since the
CODEC shutdown may need a working AC'97 register communication.

Maciej

Re: [PATCH v4 00/17] ASoC: fsl_ssi: Clean up - program flow level

[Maciej S. Szmigiero]

On 16.01.2018 00:16, Nicolin Chen wrote:
> ==Change log==
> v4
>  * Reworked the series by taking suggestions from Maciej
>   + Added TXBIT0 bit back to play safe in PATCH-14
>   + Made bool synchronous exclusive with AC97 mode in PATCH-16
> v3
>  * Reworked the series by taking suggestions from Maciej
>   + Added PATCH-01 to make RX and TX more clearly defined
>   + Replaced "bool dir" with "int dir" in PATCH-04
>   + Replaced "!dir" with "int adir" in PATCH-05
>   + Put CBM_CFS behind the baudclk check to keep the same
> program flow in PATCH-14
>   + Removed all cpu_dai_drv changes in PATCH-15
> v2
>  * Reworked the series by taking suggestions from Maciej
>   + Added PATCH-01 to keep all ssi->i2s_net updated
>   + Replaced bool tx with bool dir in PATCH-03 and PATCH-06
>   + Moved all initial register configurations from dai probe() to
> platform probe() so as to let AC97 CODEC successfully probe.
>  * Added Tested-by from Caleb for TDM test cases.
> 
> ==Background==
> The fsl_ssi driver was designed for PPC originally and then it has
> been updated to support different modes for i.MX Series, including
> SDMA, I2S Master mode, AC97 and older i.MXs with FIQ, by different
> contributors for different use cases in different coding styles.
> 
> Additionally, in order to fix/work-around hardware bugs and design
> flaws, the driver made a lot of compromise so now its program flow
> looks very complicated and it's getting hard to maintain or update.
> 
> So I am going to clean up the driver on both coding style level and
> program flow level.
> 
> ==Introduction==
> This series of patches is the second set to clean up fsl_ssi driver
> in the program flow level. Any patch here may impact a fundamental
> test case like playback or record.
> 
> ==Verification==
> This series of patches require fully tested. I have done such tests
> on i.MX6SoloX with WM8962 using imx_v6_v7_defconfig as:
>  - Playback via I2S Master and Slave mode
>  - Record via I2S Master and Slave mode
>  - Simultaneous playback and record via I2S Master and Slave mode
>  - Background playback with foreground record (starting at different
>time) via I2S Master and Slave mode
>  - Background record with foreground playback (starting at different
>time) via I2S Master and Slave mode
>  * All tests above by hacking offline_config to true in imx51.
> 
> Caleb has tested v1 with TDM lookback tests on i.MX6.
> 
> Example of uncovered tests: AC97, PowerPC and FIQ.

I've tested the whole series in the AC'97 mode on an i.MX6 UDOO board
and everything seems to work fine as long as few small changes are made
to patches 13 and 16.

Maciej

Re: [PATCH v3 16/17] ASoC: fsl_ssi: Move DT related code to a separate probe()

[Maciej S. Szmigiero]

On 15.01.2018 05:21, Nicolin Chen wrote:
> This patch cleans up probe() function by moving all Device Tree
> related code into a separate function. It allows the probe() to
> be Device Tree independent. This will be very useful for future
> integration of imx-ssi driver which has similar functionalities
> while exists only because it supports non-DT cases.
> 
> This patch also moves symmetric_channels of AC97 from the probe
> to the structure snd_soc_dai_driver for simplification.
> 
> Additionally, since PowerPC and AC97 use the same pdev pointer
> to register a platform device, this patch also unifies related
> code.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 202 
> +---
>  1 file changed, 107 insertions(+), 95 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 20889d8..d2072de 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1366,41 +1365,102 @@ static void fsl_ssi_imx_clean(struct platform_device 
> *pdev, struct fsl_ssi *ssi)
>   clk_disable_unprepare(ssi->clk);
>  }
>  
> -static int fsl_ssi_probe(struct platform_device *pdev)
> +static int fsl_ssi_probe_from_dt(struct fsl_ssi *ssi)
>  {
> - struct fsl_ssi *ssi;
> - int ret = 0;
> - struct device_node *np = pdev->dev.of_node;
> - struct device *dev = >dev;
> + struct device *dev = ssi->dev;
> + struct device_node *np = dev->of_node;
>   const struct of_device_id *of_id;
>   const char *p, *sprop;
>   const uint32_t *iprop;
> - struct resource *res;
> - void __iomem *iomem;
> - char name[64];
> - struct regmap_config regconfig = fsl_ssi_regconfig;
> + u32 dmas[4];
> + int ret;
>  
>   of_id = of_match_device(fsl_ssi_ids, dev);
>   if (!of_id || !of_id->data)
>   return -EINVAL;
>  
> - ssi = devm_kzalloc(dev, sizeof(*ssi), GFP_KERNEL);
> - if (!ssi)
> - return -ENOMEM;
> -
>   ssi->soc = of_id->data;
> - ssi->dev = dev;
> +
> + ret = of_property_match_string(np, "clock-names", "ipg");
> + /* Get error code if not found */
> + ssi->has_ipg_clk_name = ret >= 0;
>  
>   /* Check if being used in AC97 mode */
>   sprop = of_get_property(np, "fsl,mode", NULL);
> - if (sprop) {
> - if (!strcmp(sprop, "ac97-slave"))
> - ssi->dai_fmt = FSLSSI_AC97_DAIFMT;
> + if (sprop && !strcmp(sprop, "ac97-slave")) {
> + ssi->dai_fmt = FSLSSI_AC97_DAIFMT;
> +
> + ret = of_property_read_u32(np, "cell-index", >card_idx);
> + if (ret) {
> + dev_err(dev, "failed to get SSI index property\n");
> + return -EINVAL;
> + }
> + strcpy(ssi->card_name, "ac97-codec");
>   }
>  
>   /* Select DMA or FIQ */
>   ssi->use_dma = !of_property_read_bool(np, "fsl,fiq-stream-filter");
>  
> + /* In synchronous mode, STCK and STFS ports are used by RX as well */
> + if (!of_find_property(np, "fsl,ssi-asynchronous", NULL))
> + ssi->synchronous = true;

You are setting ssi->synchronous in the AC'97 mode here, the old code
didn't do that (see the next patch hunk below).

Since in the previous patch you have replaced cpu_dai_drv.symmetric_rates
with ssi->synchronous this will likely break asymmetric rate support in
the AC'97 mode, since the driver will use STCCR for programming of both
playback and capture.

The next patch in this series (17) also looks affected by this change.

> @@ -1444,24 +1500,13 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>   return ssi->irq;
>   }
>  
> - /* Set software limitations for synchronous mode */
> - if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
> - if (!fsl_ssi_is_ac97(ssi)) {
> - ssi->cpu_dai_drv.symmetric_rates = 1;
> - ssi->cpu_dai_drv.symmetric_samplebits = 1;
> - ssi->synchronous = true;
> - }

You can see it here that the old code didn't set ssi->synchronous in the
AC'97 mode.

Maciej

Re: [PATCH v3 14/17] ASoC: fsl_ssi: Clean up _fsl_ssi_set_dai_fmt()

[Maciej S. Szmigiero]

On 15.01.2018 05:21, Nicolin Chen wrote:
> The _fsl_ssi_set_dai_fmt() is a helper function being called from
> fsl_ssi_set_dai_fmt() as an ASoC operation and fsl_ssi_hw_init()
> mainly for AC97 format initialization.
> 
> This patch cleans the _fsl_ssi_set_dai_fmt() in following ways:
> * Removing *dev pointer in the parameters as it's included in the
>   *ssi pointer of struct fsl_ssi.
> * Using regmap_update_bits() instead of regmap_read() with masking
>   the value manually.
> * Removing TXBIT0 configurations since this bit is set to 1 as its
>   reset value and there is no use case so far to unset it. And it
>   is safe to remove since regmap_update_bits() won't touch it.

I didn't get any response to a comment I've written about the point
above during the previous patch iteration:> The old code set this bit in any 
mode other than AC'97 (where the
> hardware always treats this bit as set regardless of the actual value).
> I would play safe here and not rely on this bit being set by a SSI
> reset on all SSI models.

Maciej

> * Moving baudclk check to the switch-case routine to skip the I2S
>   master check. And moving SxCCR.DC settings after baudclk check.
> * Adding format settings for SND_SOC_DAIFMT_AC97 like others.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
> Changelog
> v3
>  * Put CBM_CFS behind the baudclk check to keep the same program
>flow as before
> 
>  sound/soc/fsl/fsl_ssi.c | 73 
> ++---
>  1 file changed, 33 insertions(+), 40 deletions(-)
> 

Re: [PATCH v2 14/16] ASoC: fsl_ssi: Remove cpu_dai_drv from fsl_ssi structure

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> The cpu_dai_drv is only used for symmetric_rates. So this patch replaces
> it with a synchronous boolean flag.

You make cpu_dai_drv common to all SSI instances instead of per-instance.

What if you have multiple SSIs in the system with different
symmetric_{rates,samplebits,channels} settings?

Maciej

> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 213962a..716603c 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -208,11 +208,11 @@ struct fsl_ssi_soc_data {
>   *
>   * @regs: Pointer to the regmap registers
>   * @irq: IRQ of this SSI
> - * @cpu_dai_drv: CPU DAI driver for this device
>   *
>   * @dai_fmt: DAI configuration this device is currently used with
>   * @streams: Mask of current active streams: BIT(TX) and BIT(RX)
>   * @i2s_net: I2S and Network mode configurations of SCR register
> + * @synchronous: Use synchronous mode - both of TX and RX use STCK and SFCK
>   * @use_dma: DMA is used or FIQ with stream filter
>   * @use_dual_fifo: DMA with support for dual FIFO mode
>   * @has_ipg_clk_name: If "ipg" is in the clock name list of device tree
> @@ -253,11 +253,11 @@ struct fsl_ssi_soc_data {
>  struct fsl_ssi {
>   struct regmap *regs;
>   int irq;
> - struct snd_soc_dai_driver cpu_dai_drv;
>  
>   unsigned int dai_fmt;
>   u8 streams;
>   u8 i2s_net;
> + bool synchronous;
>   bool use_dma;
>   bool use_dual_fifo;
>   bool has_ipg_clk_name;
> @@ -668,7 +668,6 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> *substream,
>   bool tx2, tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>   struct fsl_ssi *ssi = snd_soc_dai_get_drvdata(dai);
>   struct regmap *regs = ssi->regs;
> - int synchronous = ssi->cpu_dai_drv.symmetric_rates, ret;
>   u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
>   unsigned long clkrate, baudrate, tmprate;
>   unsigned int slots = params_channels(hw_params);
> @@ -676,6 +675,7 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> *substream,
>   u64 sub, savesub = 10;
>   unsigned int freq;
>   bool baudclk_is_used;
> + int ret;
>  
>   /* Override slots and slot_width if being specifically set... */
>   if (ssi->slots)
> @@ -754,7 +754,7 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> *substream,
>   mask = SSI_SxCCR_PM_MASK | SSI_SxCCR_DIV2 | SSI_SxCCR_PSR;
>  
>   /* STCCR is used for RX in synchronous mode */
> - tx2 = tx || synchronous;
> + tx2 = tx || ssi->synchronous;
>   regmap_update_bits(regs, REG_SSI_SxCCR(tx2), mask, stccr);
>  
>   if (!baudclk_is_used) {
> @@ -802,7 +802,7 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream 
> *substream,
>* that should set separate configurations for STCCR and SRCCR
>* despite running in the synchronous mode.
>*/
> - if (enabled && ssi->cpu_dai_drv.symmetric_rates)
> + if (enabled && ssi->synchronous)
>   return 0;
>  
>   if (fsl_ssi_is_i2s_master(ssi)) {
> @@ -834,7 +834,7 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream 
> *substream,
>   }
>  
>   /* In synchronous mode, the SSI uses STCCR for capture */
> - tx2 = tx || ssi->cpu_dai_drv.symmetric_rates;
> + tx2 = tx || ssi->synchronous;
>   regmap_update_bits(regs, REG_SSI_SxCCR(tx2), SSI_SxCCR_WL_MASK, wl);
>  
>   return 0;
> @@ -959,7 +959,7 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi *ssi, 
> unsigned int fmt)
>   srcr = strcr;
>  
>   /* Set SYN mode and clear RXDIR bit when using SYN or AC97 mode */
> - if (ssi->cpu_dai_drv.symmetric_rates || fsl_ssi_is_ac97(ssi)) {
> + if (ssi->synchronous || fsl_ssi_is_ac97(ssi)) {
>   srcr &= ~SSI_SRCR_RXDIR;
>   scr |= SSI_SCR_SYN;
>   }
> @@ -1360,6 +1360,7 @@ static void fsl_ssi_imx_clean(struct platform_device 
> *pdev, struct fsl_ssi *ssi)
>  
>  static int fsl_ssi_probe(struct platform_device *pdev)
>  {
> + struct snd_soc_dai_driver *cpu_dai_drv;
>   struct fsl_ssi *ssi;
>   int ret = 0;
>   struct device_node *np = pdev->dev.of_node;
> @@ -1394,14 +1395,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>   ssi->use_dma = !of_property_read_bool(np, "fsl,fiq-stream-filter");
>  
>   if (fsl_ssi_is_ac97(ssi)) {
> - memcpy(>cpu_dai_drv, _ssi_ac97_dai,
> -sizeof(fsl_ssi_ac97_dai));
> + cpu_dai_drv = _ssi_ac97_dai;
>   fsl_ac97_data = ssi;
>   } else {
> - memcpy(>cpu_dai_drv, _ssi_dai_template,
> -sizeof(fsl_ssi_dai_template));
> + cpu_dai_drv = _ssi_dai_template;
>   }
> - 

Re: [PATCH v2 13/16] ASoC: fsl_ssi: Clean up _fsl_ssi_set_dai_fmt()

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> The _fsl_ssi_set_dai_fmt() is a helper function being called from
> fsl_ssi_set_dai_fmt() as an ASoC operation and fsl_ssi_hw_init()
> mainly for AC97 format initialization.
> 
> This patch cleans the _fsl_ssi_set_dai_fmt() in following ways:
> * Removing *dev pointer in the parameters as it's included in the
>   *ssi pointer of struct fsl_ssi.
> * Using regmap_update_bits() instead of regmap_read() with masking
>   the value manually.
> * Removing TXBIT0 configurations since this bit is set to 1 as its
>   reset value and there is no use case so far to unset it. And it
>   is safe to remove since regmap_update_bits() won't touch it.

The old code set this bit in any mode other than AC'97 (where the
hardware always treats this bit as set regardless of the actual value).
I would play safe here and not rely on this bit being set by a SSI
reset on all SSI models.

> * Moving baudclk check to the switch-case routine to skip the I2S
>   master check. And moving SxCCR.DC settings after baudclk check.
> * Adding format settings for SND_SOC_DAIFMT_AC97 like others.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 70 
> ++---
>  1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 178c192..213962a 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -855,42 +855,27 @@ static int fsl_ssi_hw_free(struct snd_pcm_substream 
> *substream,
>   return 0;
>  }
>  
> -static int _fsl_ssi_set_dai_fmt(struct device *dev,
> - struct fsl_ssi *ssi, unsigned int fmt)
> +static int _fsl_ssi_set_dai_fmt(struct fsl_ssi *ssi, unsigned int fmt)
>  {
> - struct regmap *regs = ssi->regs;
> - u32 strcr = 0, stcr, srcr, scr, mask;
> + u32 strcr = 0, scr = 0, stcr, srcr, mask;
>  
>   ssi->dai_fmt = fmt;
>  
> - if (fsl_ssi_is_i2s_master(ssi) && IS_ERR(ssi->baudclk)) {
> - dev_err(dev, "missing baudclk for master mode\n");
> - return -EINVAL;
> - }
> -
> - regmap_read(regs, REG_SSI_SCR, );
> - scr &= ~(SSI_SCR_SYN | SSI_SCR_I2S_MODE_MASK);
>   /* Synchronize frame sync clock for TE to avoid data slipping */
>   scr |= SSI_SCR_SYNC_TX_FS;
>  
> - mask = SSI_STCR_TXBIT0 | SSI_STCR_TFDIR | SSI_STCR_TXDIR |
> -SSI_STCR_TSCKP | SSI_STCR_TFSI | SSI_STCR_TFSL | SSI_STCR_TEFS;
> - regmap_read(regs, REG_SSI_STCR, );
> - regmap_read(regs, REG_SSI_SRCR, );
> - stcr &= ~mask;
> - srcr &= ~mask;
> -
>   /* Use Network mode as default */
>   ssi->i2s_net = SSI_SCR_NET;
>   switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>   case SND_SOC_DAIFMT_I2S:
> - regmap_update_bits(regs, REG_SSI_STCCR,
> -SSI_SxCCR_DC_MASK, SSI_SxCCR_DC(2));
> - regmap_update_bits(regs, REG_SSI_SRCCR,
> -SSI_SxCCR_DC_MASK, SSI_SxCCR_DC(2));
>   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>   case SND_SOC_DAIFMT_CBM_CFS:
>   case SND_SOC_DAIFMT_CBS_CFS:
> + if (IS_ERR(ssi->baudclk)) {
> + dev_err(ssi->dev,
> + "missing baudclk for master mode\n");
> + return -EINVAL;
> + }

The original code did this check only for fsl_ssi_is_i2s_master(ssi),
that is, only for SND_SOC_DAIFMT_CBS_CFS while here you also do it for
SND_SOC_DAIFMT_CBM_CFS.
Was this changed on purpose?

Maciej

Re: [PATCH v2 06/16] ASoC: fsl_ssi: Clean up helper functions of trigger()

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> The trigger() calls fsl_ssi_tx_config() and fsl_ssi_rx_config(),
> and both of them jump to fsl_ssi_config(). And fsl_ssi_config()
> later calls another fsl_ssi_rxtx_config().
> 
> However, the whole routine, especially fsl_ssi_config() function,
> is too complicated because of the folowing reasons:
> 1) It has to handle the concern of the opposite stream.
> 2) It has to handle cases of offline configurations support.
> 3) It has to handle enable and disable operations while they're
>mostly different.
> 
> Since the enable and disable routines have more differences than
> TX and RX rountines, this patch simplifies these helper functions
> with the following changes:
> - Changing to two helper functions of enable and disable instead
>   of TX and RX.
> - Removing fsl_ssi_rxtx_config() by separately integrating it to
>   two newly introduced enable & disable functions.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 256 
> +++-
>  1 file changed, 122 insertions(+), 134 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 263c067..09a571a 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -378,31 +378,83 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
>  }
>  
>  /**
> - * Enable or disable all rx/tx config flags at once
> + * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
> + *
> + * Notes:
> + * 1) For offline_config SoCs, enable all necessary bits of both streams
> + *when 1st stream starts, even if the opposite stream will not start
> + * 2) It also clears FIFO before setting regvals; SOR is safe to set online
>   */
> -static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable)
> +static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool tx)
>  {
> - struct regmap *regs = ssi->regs;
>   struct fsl_ssi_regvals *vals = ssi->regvals;
> + bool dir = tx ? TX : RX;

A similar case as in patch 3 of a bool variable used for a bit index.

> + u32 sier, srcr, stcr;
>  
> - if (enable) {
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier,
> -vals[RX].sier | vals[TX].sier);
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr,
> -vals[RX].srcr | vals[TX].srcr);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr,
> -vals[RX].stcr | vals[TX].stcr);
> + /* Clear dirty data in the FIFO; It also prevents channel slipping */
> + regmap_update_bits(ssi->regs, REG_SSI_SOR,
> +SSI_SOR_xX_CLR(tx), SSI_SOR_xX_CLR(tx));
> +
> + /*
> +  * On offline_config SoCs, SxCR and SIER are already configured when
> +  * the previous stream started. So skip all SxCR and SIER settings
> +  * to prevent online reconfigurations, then jump to set SCR directly
> +  */
> + if (ssi->soc->offline_config && ssi->streams)
> + goto enable_scr;
> +
> + if (ssi->soc->offline_config) {
> + /*
> +  * Online reconfiguration not supported, so enable all bits for
> +  * both streams at once to avoid necessity of reconfigurations
> +  */
> + srcr = vals[RX].srcr | vals[TX].srcr;
> + stcr = vals[RX].stcr | vals[TX].stcr;
> + sier = vals[RX].sier | vals[TX].sier;
>   } else {
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr, 0);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr, 0);
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier, 0);
> + /* Otherwise, only set bits for the current stream */
> + srcr = vals[dir].srcr;
> + stcr = vals[dir].stcr;
> + sier = vals[dir].sier;
>   }
> +
> + /* Configure SRCR, STCR and SIER at once */
> + regmap_update_bits(ssi->regs, REG_SSI_SRCR, srcr, srcr);
> + regmap_update_bits(ssi->regs, REG_SSI_STCR, stcr, stcr);
> + regmap_update_bits(ssi->regs, REG_SSI_SIER, sier, sier);
> +
> +enable_scr:
> + /*
> +  * Start DMA before setting TE to avoid FIFO underrun
> +  * which may cause a channel slip or a channel swap
> +  *
> +  * TODO: FIQ cases might also need this upon testing
> +  */
> + if (ssi->use_dma && tx) {
> + int try = 100;
> + u32 sfcsr;
> +
> + /* Enable SSI first to send TX DMA request */
> + 

Re: [PATCH v2 04/16] ASoC: fsl_ssi: Rename fsl_ssi_disable_val macro

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> The define of fsl_ssi_disable_val is not so clear as it mixes two
> steps of calculations together. And those parameter names are also
> a bit long to read.
> 
> Since it just tries to exclude the shared bits from the regvals of
> current stream while the opposite stream is active, it's better to
> use something like ssi_excl_shared_bits.
> 
> This patch also bisects fsl_ssi_disable_val into two macros of two
> corresponding steps and then shortens its parameter names. It also
> updates callers in the fsl_ssi_config() accordingly.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 54 
> -
>  1 file changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index aa14a5d..f026386 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -445,16 +445,10 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool 
> enable,
>   bool dir = (>regvals[TX] == vals) ? TX : RX;
>   struct regmap *regs = ssi->regs;
>   struct fsl_ssi_regvals *avals;
> - int nr_active_streams;
> - int keep_active;
> -
> - nr_active_streams = !!(ssi->streams & BIT(TX)) +
> - !!(ssi->streams & BIT(RX));
> + bool aactive;
>  
> - if (nr_active_streams - 1 > 0)
> - keep_active = 1;
> - else
> - keep_active = 0;
> + /* Check if the opposite stream is active */
> + aactive = ssi->streams & BIT(!dir);
 ^
Here an implicit assumption that either RX == 0, TX == 1 or
RX == 1, TX == 0 still remains.

Maciej

Re: [PATCH v2 03/16] ASoC: fsl_ssi: Maintain a mask of active streams

[Maciej S. Szmigiero]

On 11.01.2018 07:43, Nicolin Chen wrote:
> Checking TE and RE bits in SCR register doesn't work for AC97 mode
> which enables SSIEN, TE and RE in the fsl_ssi_setup_ac97() that's
> called during probe().
> 
> So when running into the trigger(), it will always get the result
> of both TE and RE being enabled already, even if actually there is
> no active stream.
> 
> This patch fixes this issue by adding a variable to log the active
> streams manually.
> 
> Signed-off-by: Nicolin Chen 
> Tested-by: Caleb Crome 
> ---
>  sound/soc/fsl/fsl_ssi.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 491b660..aa14a5d 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -201,6 +201,7 @@ struct fsl_ssi_soc_data {
>   * @cpu_dai_drv: CPU DAI driver for this device
>   *
>   * @dai_fmt: DAI configuration this device is currently used with
> + * @streams: Mask of current active streams: BIT(TX) and BIT(RX)
>   * @i2s_net: I2S and Network mode configurations of SCR register
>   * @use_dma: DMA is used or FIQ with stream filter
>   * @use_dual_fifo: DMA with support for dual FIFO mode
> @@ -245,6 +246,7 @@ struct fsl_ssi {
>   struct snd_soc_dai_driver cpu_dai_drv;
>  
>   unsigned int dai_fmt;
> + u8 streams;
>   u8 i2s_net;
>   bool use_dma;
>   bool use_dual_fifo;
> @@ -440,15 +442,14 @@ static void fsl_ssi_fifo_clear(struct fsl_ssi *ssi, 
> bool is_rx)
>  static void fsl_ssi_config(struct fsl_ssi *ssi, bool enable,
>  struct fsl_ssi_regvals *vals)
>  {
> + bool dir = (>regvals[TX] == vals) ? TX : RX;
Using a bool variable for a bit index (and array index in other parts
of code) looks just wrong.

Even a simple int would look better IMHO here (and in patch 5 that
rewrites this line a bit).

Maciej

Re: [PATCH v1 11/15] ASoC: fsl_ssi: Setup AC97 in dai_probe()

[Maciej S. Szmigiero]

On 04.01.2018 20:07, Nicolin Chen wrote:
> On Mon, Jan 01, 2018 at 04:17:20PM +0100, Maciej S. Szmigiero wrote:
>>> AC97 configures some registers earlier to start a communication
>>> with CODECs, so this patch moves those register settings to the
>>> dai_probe() as well, along with other register configurations.
>  
>> This patch breaks AC'97 CODEC probing.
>>
>> Namely, the fsl_ssi DAI probe callback is only called after the AC'97
>> CODEC probe callback, so when you move SSI AC'97 startup to its DAI
>> probe callback it won't be done yet when the CODEC is probed (and this
>> requires a working AC'97 interface to successfully complete).
> 
> Hmm...What's the dependency here? Why is it required like this?

This patch moves enabling AC'97 communication (done by
fsl_ssi_setup_ac97() ) from SSI _platform device_ probe path to
SSI _DAI_ probe path.

However, it turns out that a SSI _DAI_ probe happens after a AC'97
CODEC probe (that is, ac97_soc_probe() in sound/soc/codecs/ac97.c).
And a AC'97 CODEC probe needs AC'97 communication to be working,
since it has to detect the CODEC model, configure it, etc.

> I am okay to put everything to a separate fsl_ssi_hw_init() and
> move it back to the platform probe() though.
> 

This could be a solution - I assume that by "everything" in the above
sentence you mean (at least) enabling the AC'97 communication at the
SSI.

Maciej

Re: [PATCH v1 05/15] ASoC: fsl_ssi: Clean up helper functions of trigger()

[Maciej S. Szmigiero]

On 19.12.2017 18:00, Nicolin Chen wrote:
> The trigger() calls fsl_ssi_tx_config() and fsl_ssi_rx_config(),
> and both of them jump to fsl_ssi_config(). And fsl_ssi_config()
> later calls another fsl_ssi_rxtx_config().
> 
> However, the whole routine, especially fsl_ssi_config() function,
> is too complicated because of the folowing reasons:
> 1) It has to handle the concern of the opposite stream.
> 2) It has to handle cases of offline configurations support.
> 3) It has to handle enable and disable operations while they're
>mostly different.
> 
> Since the enable and disable routines have more differences than
> TX and RX rountines, this patch simplifies these helper functions
> with the following changes:
> - Changing to two helper functions of enable and disable instead
>   of TX and RX.
> - Removing fsl_ssi_rxtx_config() by separately integrating it to
>   two newly introduced enable & disable functions.
> 
> Signed-off-by: Nicolin Chen 
> ---
>  sound/soc/fsl/fsl_ssi.c | 254 
> +++-
>  1 file changed, 119 insertions(+), 135 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 0f09caf..d8c8656 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -378,31 +378,81 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
>  }
>  
>  /**
> - * Enable or disable all rx/tx config flags at once
> + * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
> + *
> + * Notes:
> + * 1) For offline_config SoCs, enable all necessary bits of both streams
> + *when 1st stream starts, even if the opposite stream will not start
> + * 2) It also clears FIFO before setting regvals; SOR is safe to set online
>   */
> -static void fsl_ssi_rxtx_config(struct fsl_ssi *ssi, bool enable)
> +static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool tx)
>  {
> - struct regmap *regs = ssi->regs;
>   struct fsl_ssi_regvals *vals = ssi->regvals;
> + u32 sier, srcr, stcr;
>  
> - if (enable) {
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier,
> -vals[RX].sier | vals[TX].sier);
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr,
> -vals[RX].srcr | vals[TX].srcr);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr,
> -vals[RX].stcr | vals[TX].stcr);
> + /* Clear dirty data in the FIFO; It also prevents channel slipping */
> + regmap_update_bits(ssi->regs, REG_SSI_SOR,
> +SSI_SOR_xX_CLR(tx), SSI_SOR_xX_CLR(tx));
> +
> + /*
> +  * On offline_config SoCs, SxCR and SIER are already configured when
> +  * the previous stream started. So skip all SxCR and SIER settings
> +  * to prevent online reconfigurations, then jump to set SCR directly
> +  */
> + if (ssi->soc->offline_config && ssi->streams)
> + goto enable_scr;
> +
> + if (ssi->soc->offline_config) {
> + /*
> +  * Online reconfiguration not supported, so enable all bits for
> +  * both streams at once to avoid necessity of reconfigurations
> +  */
> + srcr = vals[RX].srcr | vals[TX].srcr;
> + stcr = vals[RX].stcr | vals[TX].stcr;
> + sier = vals[RX].sier | vals[TX].sier;
>   } else {
> - regmap_update_bits(regs, REG_SSI_SRCR,
> -vals[RX].srcr | vals[TX].srcr, 0);
> - regmap_update_bits(regs, REG_SSI_STCR,
> -vals[RX].stcr | vals[TX].stcr, 0);
> - regmap_update_bits(regs, REG_SSI_SIER,
> -vals[RX].sier | vals[TX].sier, 0);
> + /* Otherwise, only set bits for the current stream */
> + srcr = vals[tx].srcr;
> + stcr = vals[tx].stcr;
> + sier = vals[tx].sier;

Implicit assumption here that RX == 0, TX == 1, as in the 03 patch.

>   }
> +
> + /* Configure SRCR, STCR and SIER at once */
> + regmap_update_bits(ssi->regs, REG_SSI_SRCR, srcr, srcr);
> + regmap_update_bits(ssi->regs, REG_SSI_STCR, stcr, stcr);
> + regmap_update_bits(ssi->regs, REG_SSI_SIER, sier, sier);
> +
> +enable_scr:
> + /*
> +  * Start DMA before setting TE to avoid FIFO underrun
> +  * which may cause a channel slip or a channel swap
> +  *
> +  * TODO: FIQ cases might also need this upon testing
> +  */
> + if (ssi->use_dma && tx) {
> + int try = 100;
> + u32 sfcsr;
> +
> + /* Enable SSI first to send TX DMA request */
> + regmap_update_bits(ssi->regs, REG_SSI_SCR,
> +SSI_SCR_SSIEN, 

Re: [PATCH v1 03/15] ASoC: fsl_ssi: Rename fsl_ssi_disable_val macro

[Maciej S. Szmigiero]

On 19.12.2017 18:00, Nicolin Chen wrote:
> The define of fsl_ssi_disable_val is not so clear as it mixes two
> steps of calculations together. And those parameter names are also
> a bit long to read.
> 
> Since it just tries to exclude the shared bits from the regvals of
> current stream while the opposite stream is active, it's better to
> use something like ssi_excl_shared_bits.
> 
> This patch also bisects fsl_ssi_disable_val into two macros of two
> corresponding steps and then shortens its parameter names. It also
> updates callers in the fsl_ssi_config() accordingly.
> 
> Signed-off-by: Nicolin Chen 
> ---
>  sound/soc/fsl/fsl_ssi.c | 54 
> -
>  1 file changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index f05f78d..6dda2e0 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c

> @@ -445,16 +445,10 @@ static void fsl_ssi_config(struct fsl_ssi *ssi, bool 
> enable,
>   bool tx = >regvals[TX] == vals;
>   struct regmap *regs = ssi->regs;
>   struct fsl_ssi_regvals *avals;
> - int nr_active_streams;
> - int keep_active;
> -
> - nr_active_streams = !!(ssi->streams & BIT(TX)) +
> - !!(ssi->streams & BIT(RX));
> + bool aactive;
>  
> - if (nr_active_streams - 1 > 0)
> - keep_active = 1;
> - else
> - keep_active = 0;
> + /* Check if the opposite stream is active */
> + aactive = ssi->streams & BIT(!tx);

I don't think that hardcoding an implicit assumption here that RX == 0,
TX == 1 is a good thing.
If in the future, for any reason, somebody changes values of these macros
this code will silently break.

I would instead change this line into something like
"aactive = ssi->streams & (tx ? BIT(RX) : BIT(TX));" or similar.

Maciej

Re: [PATCH v1 01/15] ASoC: fsl_ssi: Clean up set_dai_tdm_slot()

[Maciej S. Szmigiero]

On 19.12.2017 18:00, Nicolin Chen wrote:
> This patch replaces the register read with ssi->i2s_net for
> simplification. It also removes masking SSIEN from scr value
> since it's handled later by regmap_update_bits() to set this
> scr value back.
> 
> Signed-off-by: Nicolin Chen 
> ---
>  sound/soc/fsl/fsl_ssi.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index aecd00f..ed2712b 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -1051,9 +1051,7 @@ static int fsl_ssi_set_dai_tdm_slot(struct snd_soc_dai 
> *dai, u32 tx_mask,
>   }
>  
>   /* The slot number should be >= 2 if using Network mode or I2S mode */
> - regmap_read(regs, REG_SSI_SCR, );
> - val &= SSI_SCR_I2S_MODE_MASK | SSI_SCR_NET;
> - if (val && slots < 2) {
> + if (ssi->i2s_net && slots < 2) {
>   dev_err(dai->dev, "slot number should be >= 2 in I2S or NET\n");
>   return -EINVAL;
>   }

Are you sure that ssi->i2s_net SSI_SCR_I2S_MODE_MASK | SSI_SCR_NET bits
(also known as SSI_SCR_I2S_NET_MASK) zero or non-zero status is always
consistent with that in the SCR register?

I can see that in fsl_ssi_hw_params() these bits in SCR are zeroed in
a one special case and in the second special case they are hardcoded
to SSI_SCR_I2S_MODE_NORMAL | SSI_SCR_NET, in both cases regardless of
what is currently in ssi->i2s_net.

Maciej

Re: [PATCH v1 11/15] ASoC: fsl_ssi: Setup AC97 in dai_probe()

[Maciej S. Szmigiero]

On 19.12.2017 18:00, Nicolin Chen wrote:
> AC97 configures some registers earlier to start a communication
> with CODECs, so this patch moves those register settings to the
> dai_probe() as well, along with other register configurations.
> 
> It also applies _fsl_ssi_set_dai_fmt() to AC97 only since other
> formats would be configured via fsl_ssi_set_dai_fmt() directly.
> 
> Meanwhile, this patch adds fsl_ssi_dai_ac97_remove() to cleanup
> some control bits for AC97.
> 
> Signed-off-by: Nicolin Chen 

This patch breaks AC'97 CODEC probing.

Namely, the fsl_ssi DAI probe callback is only called after the AC'97
CODEC probe callback, so when you move SSI AC'97 startup to its DAI
probe callback it won't be done yet when the CODEC is probed (and this
requires a working AC'97 interface to successfully complete).

Maciej

Re: [PATCH v2 00/11] ASoC: fsl_ssi: Clean up - coding style level

[Maciej S. Szmigiero]

On 13.12.2017 07:34, Nicolin Chen wrote:
> ==Changelog==
> v1->v2
>  * Dropped one patch to remove "struct device"
>  * Revised PATCH-03 "Refine all comments"
>  * Revised PATCH-05 "Refine indentations and wrappings"
>  * Rebased all other patches
>  * Added PATCH-10 "Rename i2smode to i2s_net"
>  * Added PATCH-11 "Define ternary macros to simplify code"
> 
>  # Detialed changes are described in each updated patch.
> 
> ==Background==
> The fsl_ssi driver was designed for PPC originally and then it has
> been updated to support different modes for i.MX Series, including
> SDMA, I2S Master mode, AC97 and older i.MXs with FIQ, by different
> contributors for different use cases in different coding styles.
> 
> Additionally, in order to fix/work-around hardware bugs and design
> flaws, the driver made a lot of compromise so now its program flow
> looks very complicated and it's getting hard to maintain or update.
> 
> So I am going to clean up the driver on both coding style level and
> program flow level.
> 
> ==Introduction==
> This series of patches is the first set to clean up fsl_ssi driver
> in the coding style level. Any patch here is not supposed to change
> the program flow.
> 
> ==Verification==
> Theoretically, since these patches do not change program flow, they
> only need code review, build or sanity tests. I have done build and
> sanity tests on an i.MX6SoloX with WM8962 using imx_v6_v7_defconfig
> and playback/record tests in I2S Master/Slave modes.
> 
> Nicolin Chen (11):
>   ASoC: fsl_ssi: Rename fsl_ssi_private to fsl_ssi
>   ASoC: fsl_ssi: Cache pdev->dev pointer
>   ASoC: fsl_ssi: Refine all comments
>   ASoC: fsl_ssi: Rename registers and fields macros
>   ASoC: fsl_ssi: Refine indentations and wrappings
>   ASoC: fsl_ssi: Refine printk outputs
>   ASoC: fsl_ssi: Rename cpu_dai parameter to dai
>   ASoC: fsl_ssi: Rename scr_val to scr
>   ASoC: fsl_ssi: Replace fsl_ssi_rxtx_reg_val with fsl_ssi_regvals
>   ASoC: fsl_ssi: Rename i2smode to i2s_net
>   ASoC: fsl_ssi: Define ternary macros to simplify code
> 
>  sound/soc/fsl/fsl_ssi.c | 1373 
> +++--------
>  sound/soc/fsl/fsl_ssi.h |  427 ++++------
>  sound/soc/fsl/fsl_ssi_dbg.c |   59 +-
>  3 files changed, 876 insertions(+), 983 deletions(-)
> 

For the whole series:
Tested-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
Reviewed-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>

Thanks.

Re: [PATCH v2 05/11] ASoC: fsl_ssi: Refine indentations and wrappings

[Maciej S. Szmigiero]

On 13.12.2017 07:34, Nicolin Chen wrote:
> This patch just simply unifies the coding style.
> 
> Signed-off-by: Nicolin Chen 
> ---
> 
> Changelog
> v1->v2
>  * Added two missing indentation changes
>  * Removed two extra blank lines.
> 
>  sound/soc/fsl/fsl_ssi.c | 239 
> +---
>  sound/soc/fsl/fsl_ssi.h |   2 +-
>  sound/soc/fsl/fsl_ssi_dbg.c |   3 +-
>  3 files changed, 118 insertions(+), 126 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 8b5407d..9a3db08 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
(..)
> @@ -916,12 +917,11 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
>   case SND_SOC_DAIFMT_DSP_A:
>   /* Data on rising edge of bclk, frame high, 1clk before data */
>   strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP |
> - SSI_STCR_TXBIT0 | SSI_STCR_TEFS;
> +  SSI_STCR_TXBIT0 | SSI_STCR_TEFS;
>   break;
>   case SND_SOC_DAIFMT_DSP_B:
>   /* Data on rising edge of bclk, frame high */
> - strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP |
> - SSI_STCR_TXBIT0;
> + strcr |= SSI_STCR_TFSL | SSI_STCR_TSCKP |  SSI_STCR_TXBIT0;

It looks like an extra space got here^.

Maciej

Re: [PATCH v2 03/11] ASoC: fsl_ssi: Refine all comments

[Maciej S. Szmigiero]

On 13.12.2017 07:34, Nicolin Chen wrote:
> This patch refines the comments by:
> 1) Removing all out-of-date comments
> 2) Removing all not-so-useful comments
> 3) Unifying the styles of all comments
> 4) Simplifying over-descriptive comments
> 5) Adding comments to improve code readablity
> 6) Moving all register related comments to fsl_ssi.h
> 7) Adding comments to all register and field defines
> 
> Signed-off-by: Nicolin Chen 
> ---
> 
> Changelog
> v1->v2
>  * Added some new comments at "SoC specific data" to be more precise.
>  * Revised one comment in fsl_ssi_config().
>  * Revised the comment of fsl_ssi_setup_reg_vals().
>  * Added one comment for AC97 in fsl_ssi_setup_reg_vals().
>  * Revised the comment of fsl_ssi_hw_params() to be more precise.
>  * Added some comments in _fsl_ssi_set_dai_fmt() to help understand the 
> formats.
>  * Added one comment in fsl_ssi_set_dai_fmt() to state why AC97 needs to 
> bypass it.
>  * Revised comments in fsl_ssi_set_dai_tdm_slot() to be more precise.
>  * Revised comments around dual FIFO code in fsl_ssi_imx_probe() to be more 
> precise.
> 
>  sound/soc/fsl/fsl_ssi.c | 376 
> ++--
>  sound/soc/fsl/fsl_ssi.h |  67 +++-
>  sound/soc/fsl/fsl_ssi_dbg.c |  12 +-
>  3 files changed, 188 insertions(+), 267 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index e903c92..796a7ea 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
(..)
> @@ -878,10 +794,7 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream 
> *substream,
>   regmap_read(regs, CCSR_SSI_SCR, _val);
>   enabled = scr_val & CCSR_SSI_SCR_SSIEN;
>  
> - /*
> -  * If we're in synchronous mode, and the SSI is already enabled,
> -  * then STCCR is already set properly.
> -  */
> + /* To support simultaneous TX and RX, bypass it if SSI is enabled */
>   if (enabled && ssi->cpu_dai_drv.symmetric_rates)
>   return 0;
>  

I would say that the old comment described better what is happening -
since in synchronous (& non-AC'97) mode STCCR controls both TX and RX
the first direction that is started (either playback or capture) is the
only direction that needs to configure SSI in hw_params().

When the opposite direction enters hw_params() (while the first one is
still running) there is no need to do anything and we can exit early.

Maciej

Re: [PATCH 00/10] ASoC: fsl_ssi: Clean up - coding style level

[Maciej S. Szmigiero]

On 05.12.2017 20:33, Nicolin Chen wrote:
> On Tue, Dec 05, 2017 at 02:01:17PM +0100, Maciej S. Szmigiero wrote:
> 
>>> ==Verification==
>>> Theoretically, it only needs code review, build and sanity tests. I
>>> have done build and sanity tests on an i.MX 6 platform by building
>>> using imx_v6_v7_defconfig and testing with I2S slave/master mode.
> 
>> Due to my current workload I will be able to go through this and test
>> the AC'97 mode on a UDOO board only in about a week.
> 
> This is the first round which doesn't touch program flow. If you
> prefer to verify it other than merely reviewing it, I would like
> to wait for your Tested-by. There will be another set to simplify
> all functions and their program flows after this set gets merged.
> That one will need to be fully tested.

If you can wait a week then naturally it would be better to test
these changes on a real board.

Maciej

Re: [PATCH 00/10] ASoC: fsl_ssi: Clean up - coding style level

[Maciej S. Szmigiero]

Hi Nicolin,

On 04.12.2017 21:46, Nicolin Chen wrote:
> ==Background==
> The fsl_ssi driver was designed for PPC originally and then it has
> been updated to support different modes for i.MX Series, including
> SDMA, I2S Master mode, AC97 and older i.MXs with FIQ, by different
> contributors for different use cases in different coding styles.
> 
> Additionally, in order to fix/work-around hardware bugs and design
> flaws, the driver made a lot of compromise so now its program flow
> looks very complicated and it's getting hard to maintain or update.
> 
> So I am going to clean up the driver in both coding style level and
> program flow level.
> 
> ==Introduction==
> This series of patches is the first set to clean up fsl_ssi driver
> in the coding style level. Any patch here is not supposed to change
> the program flow.
> 
> ==Verification==
> Theoretically, it only needs code review, build and sanity tests. I
> have done build and sanity tests on an i.MX 6 platform by building
> using imx_v6_v7_defconfig and testing with I2S slave/master mode.

Thanks for your cleanup work.

Due to my current workload I will be able to go through this and test
the AC'97 mode on a UDOO board only in about a week.

Maciej

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

On 01.12.2017 00:53, Nicolin Chen wrote:
> On Thu, Nov 30, 2017 at 08:20:08PM +0100, Maciej S. Szmigiero wrote:
> 
>> In the AC'97 mode we have to differentiate two things:
>> 1) Bit width of the physical AC'97 interface ("AC-link"),
>> 2) Bit width of samples that are accepted during a playback and output
>> during a recording by the SSI (and, so by the sound card that it driven
>> by this CPU).
>>
>> Bit width of the physical AC'97 interface is fixed at 20 bits per sample
>> (both in playback and capture direction).
>>
>> Bit width of samples that are accepted (or produced) by the SSI could
>> be set in its STCCR and SRCCR registers (although in the AC'97 mode
>> only settings of either 16 or 20 bits are possible).
>> Each direction could be set independently from the other one.
> 
> I checked the reference manual regarding the AC97 configurations.
> It seems that AC97 sets SYNC bit in SCR register. However, unlike
> I2S which only uses STCCR for both TX and RX, AC97 does use STCCR
> and SRCCR separately. So bypassing SRCCR if SYNC bit is set isn't
> correct for AC97 cases.

I assume by 'SYNC' bit you mean the 'SYN' bit, not the 'SYNC_TX_FS' one.

You are right, we can't ignore SRCCR only because 'SYN' is set (at least
not in the AC'97 mode).
That's why the current driver doesn't do it this way, the
'symmetric_rates' flag is used instead.

> 
> I will clean up the driver a bit and I think the change would be
> highly related to AC97 code. So I'll later need you review/test.

OK.

>From my perspective it would be great if the whole cleanup was in one
series, so the whole testing doesn't need to be repeated per patch
(it involves a lot of manual work).

>> Regarding a sample rate in AC'97 mode its effective value isn't really
>> controlled by the CPU (that is, SSI), but by a CODEC since it is
>> the CODEC which tells the CPU when it should send a next sample for
>> playback and when a next capture sample is ready.
>> There are no problems if they are different (as long as the CODEC
>> supports this, naturally, but it's up to its driver to restrict the
>> sample rate space accordingly).
> 
> It's because CODEC drives the bit clock and framesync clock, isn't
> it? 

Strictly speaking, the frame sync is driven by the controller (SSI),
but it is simply the CODEC-provided bit clock divided by 256.
And the CODEC-provided bit clock is fixed at 12.288MHz by the AC'97
specs.

But every frame from CODEC also has 'TAG' bits which tell the
controller whether this frame contains valid capture samples or not.
If the capture sample rate currently programmed in CODEC is less
than 48kHz (the frame rate) it simply means that some of incoming
frames will contain 'TAG' bits indicating that these frames do not
contain valid capture samples (for example, if the capture rate is
24kHz then only half of the frames, on average, will be marked by CODEC
as containing valid capture samples).

The situation with playback is similar: the frame from CODEC also has
'SLOTREQ' bits which tell the controller if it should send playback
samples (and which) in the next frame - for example, if the playback
rate is 24kHz then in half of the frames, on average, the CODEC will
request playback samples.

Hope it is clear now.

> Could SSI act as the clock master side? It doesn't seem to be
> configurable to do so according to the reference manual though.

In the AC'97 mode?
I couldn't find in the AC'97 specs any reference to a mode of
operation in which the controller generates the bit clock.

Also, the SSI manual (from imx6q) states that in the AC'97 mode:
"TXDIR bit (SSI.STCR[5]) is forced to 0 internally to select external
generated bit clock".
This suggests it isn't possible to make the SSI generate the bit clock
in this mode of operation.

> 
>> A comment above "fsl,ssi-asynchronous" property says that it is about
>> whether "the RX and the TX clocks [are] locked".
>> They are on the physical AC'97 interface, but they aren't on the logical
>> playback / capture interface.
>> IMHO, this configurable property simply makes no sense in the AC'97
>> mode.
> 
> The property is more on the hardware level. And we should refer to
> the DT binding doc:
> fsl,ssi-asynchronous:
>  If specified, the SSI is to be programmed in asynchronous
>  mode.  In this mode, pins SRCK, STCK, SRFS, and STFS must
>  all be connected to valid signals.  In synchronous mode,
>  SRCK and SRFS are ignored.  Asynchronous mode allows
>  playback and capture to use different sample sizes and
>  sample rates.  Some drivers may require that SRCK and STCK
>  be connected together, and SRFS and STFS be connected
>  together.  This would still allow different sample sizes,
>  but not different sample rates.
> 
> If AC97 doesn't need SRFS and SRCK at all, it's fine to ignore it.

Yes, the AC'97 mode doesn't use SRFS and SRCK at all.

> Thanks
> Nic

Best regards,
Maciej

Re: [PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

Hi Nicolin,

On 30.11.2017 08:23, Nicolin Chen wrote:
> Hi Maciej,
> 
> On Mon, Nov 27, 2017 at 11:34:44PM +0100, Maciej S. Szmigiero wrote:
>> There is no problem in using different bit widths in playback and capture
>> in AC'97 mode so allow this, too.
> 
>> @@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device 
>> *pdev)
>>  
>>  /* Are the RX and the TX clocks locked? */
>>  if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
>> -if (!fsl_ssi_is_ac97(ssi_private))
>> +if (!fsl_ssi_is_ac97(ssi_private)) {
>>  ssi_private->cpu_dai_drv.symmetric_rates = 1;
>> +ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>> +}
>>  
>>  ssi_private->cpu_dai_drv.symmetric_channels = 1;
>> -ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
>>  }
> 
> I was actually wondering how the AC97 works in the synchronous mode
> while being able to handle different bit widths. Then I found that
> the drivers does corresponding configurations for synchronous mode
> only if symmetric_rates is set -- which is unset for AC97 cases. So
> in fact AC97 case (symmetric_rates unset) is probably being treated
> as asynchronous mode by the driver -- it'd be better if you confirm
> this for me.
> 
> And I am not so sure about the physical pin connections in an AC97
> situation, but I started to think that, instead of having a change
> above, AC97 cases might be supposed to have "fsl,ssi-asynchronous"
> property in DT since it's working when the driver sets both TX and
> RX control registers (i.e. asynchronous mode), not like synchronous
> mode that only sets TX's registers
In the AC'97 mode we have to differentiate two things:
1) Bit width of the physical AC'97 interface ("AC-link"),
2) Bit width of samples that are accepted during a playback and output
during a recording by the SSI (and, so by the sound card that it driven
by this CPU).

Bit width of the physical AC'97 interface is fixed at 20 bits per sample
(both in playback and capture direction).

Bit width of samples that are accepted (or produced) by the SSI could
be set in its STCCR and SRCCR registers (although in the AC'97 mode
only settings of either 16 or 20 bits are possible).
Each direction could be set independently from the other one.

That's what the driver configures depending on what an ALSA application
had requested.

Regarding a sample rate in AC'97 mode its effective value isn't really
controlled by the CPU (that is, SSI), but by a CODEC since it is
the CODEC which tells the CPU when it should send a next sample for
playback and when a next capture sample is ready.
There are no problems if they are different (as long as the CODEC
supports this, naturally, but it's up to its driver to restrict the
sample rate space accordingly).

A comment above "fsl,ssi-asynchronous" property says that it is about
whether "the RX and the TX clocks [are] locked".
They are on the physical AC'97 interface, but they aren't on the logical
playback / capture interface.
IMHO, this configurable property simply makes no sense in the AC'97
mode.

> Thanks
> Nicolin
> 

Best regards,
Maciej

[PATCH v4 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

When testing AC'97 capture on UDOO board (currently the only user of
fsl_ssi driver in the AC'97 mode) it become obvious that there is a massive
distortion above certain, small input signal.

This problem has been traced to silicon errata ERR003778:
"In AC97, 16-bit mode, received data is shifted by 4-bit locations" that
has "No fix scheduled".
This errata suggests a workaround of doing a 4-bit shift back in SDMA
script for this specific operation mode, however our SDMA scripts are
shared between various SoC peripherals so we can't really modify them.

There is a simple way to avoid this problem, however, that is to disallow
recording in 16-bit mode and only support it in AC'97-native 20-bit mode.
We have to use a 4-byte format for this since SSI FIFOs do not allow 3-byte
accesses (and these aren't supported by imx-sdma driver anyway).
With this change the capture distortion is gone.

We can also add this format as an additional one supported for playback,
using this opportunity to make sure that we use CPU-endian-native formats
in AC'97 mode as we already do in I2S mode.

There is no problem in using different bit widths in playback and capture
in AC'97 mode so allow this, too.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Adapt format name to changes in the first patch from
this series.

Changes from v2, v3: None.

 sound/soc/fsl/fsl_ssi.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 20ef09e1a395..c350117c8e31 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1278,14 +1278,15 @@ static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_8000_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   .formats = SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S20,
},
.capture = {
.stream_name = "AC97 Capture",
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   /* 16-bit capture is broken (errata ERR003778) */
+   .formats = SNDRV_PCM_FMTBIT_S20,
},
.ops = _ssi_dai_ops,
 };
@@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
-   if (!fsl_ssi_is_ac97(ssi_private))
+   if (!fsl_ssi_is_ac97(ssi_private)) {
ssi_private->cpu_dai_drv.symmetric_rates = 1;
+   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
+   }
 
ssi_private->cpu_dai_drv.symmetric_channels = 1;
-   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
}
 
/* Determine the FIFO depth. */

[PATCH v4 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20

[Maciej S. Szmigiero]

This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte (more conventional)
format is needed for it.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Drop "_4" suffix from these formats since they aren't
non-standard ones, use empty format slots starting from format number 25
for them, add information that they are LSB justified formats.

Changes from v2: Adapt a comment in sound/core/pcm_misc.c so it still
refers to the same sample formats as before.

Changes from v3: Make the comment style in include/uapi/sound/asound.h
match the style of comments at other sample formats.

Corresponding alsa-lib changes will be posted as soon as this patch is
merged on the kernel side, to keep alsa-lib and kernel synchronized.

 include/sound/pcm.h |  8 
 include/sound/soc-dai.h |  2 ++
 include/uapi/sound/asound.h |  9 +
 sound/core/pcm_misc.c   | 19 ++-
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 24febf9e177c..e054c583d3b3 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -169,6 +169,10 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_IMA_ADPCM _SNDRV_PCM_FMTBIT(IMA_ADPCM)
 #define SNDRV_PCM_FMTBIT_MPEG  _SNDRV_PCM_FMTBIT(MPEG)
 #define SNDRV_PCM_FMTBIT_GSM   _SNDRV_PCM_FMTBIT(GSM)
+#define SNDRV_PCM_FMTBIT_S20_LE_SNDRV_PCM_FMTBIT(S20_LE)
+#define SNDRV_PCM_FMTBIT_U20_LE_SNDRV_PCM_FMTBIT(U20_LE)
+#define SNDRV_PCM_FMTBIT_S20_BE_SNDRV_PCM_FMTBIT(S20_BE)
+#define SNDRV_PCM_FMTBIT_U20_BE_SNDRV_PCM_FMTBIT(U20_BE)
 #define SNDRV_PCM_FMTBIT_SPECIAL   _SNDRV_PCM_FMTBIT(SPECIAL)
 #define SNDRV_PCM_FMTBIT_S24_3LE   _SNDRV_PCM_FMTBIT(S24_3LE)
 #define SNDRV_PCM_FMTBIT_U24_3LE   _SNDRV_PCM_FMTBIT(U24_3LE)
@@ -202,6 +206,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_LE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_LE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE
+#define SNDRV_PCM_FMTBIT_S20   SNDRV_PCM_FMTBIT_S20_LE
+#define SNDRV_PCM_FMTBIT_U20   SNDRV_PCM_FMTBIT_U20_LE
 #endif
 #ifdef SNDRV_BIG_ENDIAN
 #define SNDRV_PCM_FMTBIT_S16   SNDRV_PCM_FMTBIT_S16_BE
@@ -213,6 +219,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_BE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_BE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE
+#define SNDRV_PCM_FMTBIT_S20   SNDRV_PCM_FMTBIT_S20_BE
+#define SNDRV_PCM_FMTBIT_U20   SNDRV_PCM_FMTBIT_U20_BE
 #endif
 
 struct snd_pcm_file {
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 58acd00cae19..d970879944fc 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -102,6 +102,8 @@ struct snd_compr_stream;
   SNDRV_PCM_FMTBIT_S16_BE |\
   SNDRV_PCM_FMTBIT_S20_3LE |\
   SNDRV_PCM_FMTBIT_S20_3BE |\
+  SNDRV_PCM_FMTBIT_S20_LE |\
+  SNDRV_PCM_FMTBIT_S20_BE |\
   SNDRV_PCM_FMTBIT_S24_3LE |\
   SNDRV_PCM_FMTBIT_S24_3BE |\
SNDRV_PCM_FMTBIT_S32_LE |\
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..07d61583fd02 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -214,6 +214,11 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_IMA_ADPCM  ((__force snd_pcm_format_t) 22)
 #defineSNDRV_PCM_FORMAT_MPEG   ((__force snd_pcm_format_t) 23)
 #defineSNDRV_PCM_FORMAT_GSM((__force snd_pcm_format_t) 24)
+#defineSNDRV_PCM_FORMAT_S20_LE ((__force snd_pcm_format_t) 25) /* in 
four bytes, LSB justified */
+#defineSNDRV_PCM_FORMAT_S20_BE ((__force snd_pcm_format_t) 26) /* in 
four bytes, LSB justified */
+#defineSNDRV_PCM_FORMAT_U20_LE ((__force snd_pcm_format_t) 27) /* in 
four bytes, LSB justified */
+#defineSNDRV_PCM_FORMAT_U20_BE ((__force snd_pcm_format_t) 28) /* in 
four bytes, LSB justified */
+/* gap in the numbering for a future standard linear format */
 #defineSNDRV_PCM_FORMAT_SPECIAL((__force snd_pcm_format_t) 31)
 #defineSNDRV_PCM_FORMAT_S24_3LE((__force snd_pcm_format_t) 32) 
/* in three bytes */
 #defineSNDRV_PCM_FORMAT_S24_3BE((__force snd_pcm_format_t) 33) 
/* in three bytes */
@@ -248,6 +253,8 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_FLOAT  SNDRV_PCM_FORMAT_FLOAT_LE
 #defineSNDRV_PC

Re: [alsa-devel] [PATCH v3 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20

[Maciej S. Szmigiero]

On 27.11.2017 21:28, Takashi Iwai wrote:
> On Mon, 27 Nov 2017 00:09:47 +0100,
> Maciej S. Szmigiero wrote:
>> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
>> index 58acd00cae19..d970879944fc 100644
>> --- a/include/sound/soc-dai.h
>> +++ b/include/sound/soc-dai.h
>> @@ -102,6 +102,8 @@ struct snd_compr_stream;
>> SNDRV_PCM_FMTBIT_S16_BE |\
>> SNDRV_PCM_FMTBIT_S20_3LE |\
>> SNDRV_PCM_FMTBIT_S20_3BE |\
>> +   SNDRV_PCM_FMTBIT_S20_LE |\
>> +   SNDRV_PCM_FMTBIT_S20_BE |\
>> SNDRV_PCM_FMTBIT_S24_3LE |\
>> SNDRV_PCM_FMTBIT_S24_3BE |\
>> SNDRV_PCM_FMTBIT_S32_LE |\
> 
> Is it really safe to include them unconditionally...?

This define is used as a template of supported formats only in ASoC AC'97
CODECs.
A list of effective supported sample formats will be calculated as an
intersection of CODEC and CPU-supported formats (and DMA
controller-supported bit widths).

This means that as long as ASoC CPUs don't add these sample formats to
their list of supported formats they should not be offered as supported
by any sound card.

Also, we already have similar SNDRV_PCM_FMTBIT_S20_3 formats in this
define.

>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index c227ccba60ae..7385024041d2 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -214,6 +214,11 @@ typedef int __bitwise snd_pcm_format_t;
>>  #define SNDRV_PCM_FORMAT_IMA_ADPCM  ((__force snd_pcm_format_t) 22)
>>  #define SNDRV_PCM_FORMAT_MPEG   ((__force snd_pcm_format_t) 23)
>>  #define SNDRV_PCM_FORMAT_GSM((__force snd_pcm_format_t) 24)
>> +#define SNDRV_PCM_FORMAT_S20_LE ((__force snd_pcm_format_t) 25) /* \
>> */
>> +#define SNDRV_PCM_FORMAT_S20_BE ((__force snd_pcm_format_t) 26) /* |
>> */
>> +#define SNDRV_PCM_FORMAT_U20_LE ((__force snd_pcm_format_t) 27) /* | in 
>> four bytes, */
>> +#define SNDRV_PCM_FORMAT_U20_BE ((__force snd_pcm_format_t) 28) /* / 
>> LSB justified  */
> 
> This style of comments is unusual, so I prefer rather a dumb style,
> even don't mind repeating the same text in each line.  They'll be over
> 80 chars in anyway, so ignore what checkpatch complains.

OK, will change it to the repetitive style then in a respin.

> thanks,
> 
> Takashi
> 

Thanks,
Maciej

Re: [PATCH v3 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20

[Maciej S. Szmigiero]

On 27.11.2017 18:40, Takashi Sakamoto wrote:
(..)
> 
> Looks good to me.
> 
> Reviewed-by: Takashi Sakamoto 

Thanks.

> In next time to post any of your v2 patchset, it's better to add commenters 
> of v1 patchset to CC list, so that your patch reaches the person who has 
> practical interests in it.

Will do it the next time.

> Thanks
> 
> Takashi Sakamoto

Best regards,
Maciej Szmigiero

Re: [PATCH v2 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20

[Maciej S. Szmigiero]

Hi,

On 26.11.2017 10:27, Takashi Sakamoto wrote:
> Hi,
> 
(..)
> Before applying this patch:
> 166 /* FIXME: the following three formats are not defined properly 
> yet */
> 167 [SNDRV_PCM_FORMAT_MPEG] = {
> 168 .le = -1, .signd = -1,
> 169 },
> 170 [SNDRV_PCM_FORMAT_GSM] = {
> 171 .le = -1, .signd = -1,
> 172 },
> 173 [SNDRV_PCM_FORMAT_SPECIAL] = {
> 174 .le = -1, .signd = -1,
> 175 },
> 
> After applying this patch:
> 
> 166 /* FIXME: the following three formats are not defined properly 
> yet */
> 167 [SNDRV_PCM_FORMAT_MPEG] = {
> 168 .le = -1, .signd = -1,
> 169 },
> 170 [SNDRV_PCM_FORMAT_GSM] = {
> 171 .le = -1, .signd = -1,
> 172 },
> 173 [SNDRV_PCM_FORMAT_S20_LE] = {
(..)> 
> I think it good to add an alternative comment for each of entry which is not 
> defined yet, like:
> 
> -> 166 /* FIXME: this format is not defined properly yet */
> 167 [SNDRV_PCM_FORMAT_MPEG] = {
> 168 .le = -1, .signd = -1,
> 169 },
> -> 170 /* FIXME: this format is not defined properly yet */
> 171 [SNDRV_PCM_FORMAT_GSM] = {
> 172 .le = -1, .signd = -1,
> 173 },
> 174 [SNDRV_PCM_FORMAT_S20_LE] = {
> 175 .width = 20, .phys = 32, .le = 1, .signd = 1,
> 176 .silence = {},
> 177 },
> 178 [SNDRV_PCM_FORMAT_S20_BE] = {
> 179 .width = 20, .phys = 32, .le = 0, .signd = 1,
> 180 .silence = {},
> 181 },
> 182 [SNDRV_PCM_FORMAT_U20_LE] = {
> 183 .width = 20, .phys = 32, .le = 1, .signd = 0,
> 184 .silence = { 0x00, 0x00, 0x08, 0x00 },
> 185 },
> 186 [SNDRV_PCM_FORMAT_U20_BE] = {
> 187 .width = 20, .phys = 32, .le = 0, .signd = 0,
> 188 .silence = { 0x00, 0x08, 0x00, 0x00 },
> 189 },
> -> 190 /* FIXME: this format is not defined properly yet */
> 191 [SNDRV_PCM_FORMAT_SPECIAL] = {
> 192 .le = -1, .signd = -1,
> 193 },
> 

Thanks, fixed now in v3.

> Regards
> 
> Takashi Sakamoto

Best regards,
Maciej Szmigiero

[PATCH v3 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

When testing AC'97 capture on UDOO board (currently the only user of
fsl_ssi driver in the AC'97 mode) it become obvious that there is a massive
distortion above certain, small input signal.

This problem has been traced to silicon errata ERR003778:
"In AC97, 16-bit mode, received data is shifted by 4-bit locations" that
has "No fix scheduled".
This errata suggests a workaround of doing a 4-bit shift back in SDMA
script for this specific operation mode, however our SDMA scripts are
shared between various SoC peripherals so we can't really modify them.

There is a simple way to avoid this problem, however, that is to disallow
recording in 16-bit mode and only support it in AC'97-native 20-bit mode.
We have to use a 4-byte format for this since SSI FIFOs do not allow 3-byte
accesses (and these aren't supported by imx-sdma driver anyway).
With this change the capture distortion is gone.

We can also add this format as an additional one supported for playback,
using this opportunity to make sure that we use CPU-endian-native formats
in AC'97 mode as we already do in I2S mode.

There is no problem in using different bit widths in playback and capture
in AC'97 mode so allow this, too.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Adapt format name to changes in the first patch from
this series.

Changes from v2: None.

 sound/soc/fsl/fsl_ssi.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 20ef09e1a395..c350117c8e31 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1278,14 +1278,15 @@ static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_8000_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   .formats = SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S20,
},
.capture = {
.stream_name = "AC97 Capture",
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   /* 16-bit capture is broken (errata ERR003778) */
+   .formats = SNDRV_PCM_FMTBIT_S20,
},
.ops = _ssi_dai_ops,
 };
@@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
-   if (!fsl_ssi_is_ac97(ssi_private))
+   if (!fsl_ssi_is_ac97(ssi_private)) {
ssi_private->cpu_dai_drv.symmetric_rates = 1;
+   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
+   }
 
ssi_private->cpu_dai_drv.symmetric_channels = 1;
-   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
}
 
/* Determine the FIFO depth. */

[PATCH v3 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20

[Maciej S. Szmigiero]

This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte (more conventional)
format is needed for it.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Drop "_4" suffix from these formats since they aren't
non-standard ones, use empty format slots starting from format number 25
for them, add information that they are LSB justified formats.

Changes from v2: Adapt a comment in sound/core/pcm_misc.c so it still
refers to the same sample formats as before.

Corresponding alsa-lib changes will be posted as soon as this patch is
merged on the kernel side, to keep alsa-lib and kernel synchronized.

 include/sound/pcm.h |  8 
 include/sound/soc-dai.h |  2 ++
 include/uapi/sound/asound.h |  9 +
 sound/core/pcm_misc.c   | 19 ++-
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 24febf9e177c..e054c583d3b3 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -169,6 +169,10 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_IMA_ADPCM _SNDRV_PCM_FMTBIT(IMA_ADPCM)
 #define SNDRV_PCM_FMTBIT_MPEG  _SNDRV_PCM_FMTBIT(MPEG)
 #define SNDRV_PCM_FMTBIT_GSM   _SNDRV_PCM_FMTBIT(GSM)
+#define SNDRV_PCM_FMTBIT_S20_LE_SNDRV_PCM_FMTBIT(S20_LE)
+#define SNDRV_PCM_FMTBIT_U20_LE_SNDRV_PCM_FMTBIT(U20_LE)
+#define SNDRV_PCM_FMTBIT_S20_BE_SNDRV_PCM_FMTBIT(S20_BE)
+#define SNDRV_PCM_FMTBIT_U20_BE_SNDRV_PCM_FMTBIT(U20_BE)
 #define SNDRV_PCM_FMTBIT_SPECIAL   _SNDRV_PCM_FMTBIT(SPECIAL)
 #define SNDRV_PCM_FMTBIT_S24_3LE   _SNDRV_PCM_FMTBIT(S24_3LE)
 #define SNDRV_PCM_FMTBIT_U24_3LE   _SNDRV_PCM_FMTBIT(U24_3LE)
@@ -202,6 +206,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_LE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_LE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE
+#define SNDRV_PCM_FMTBIT_S20   SNDRV_PCM_FMTBIT_S20_LE
+#define SNDRV_PCM_FMTBIT_U20   SNDRV_PCM_FMTBIT_U20_LE
 #endif
 #ifdef SNDRV_BIG_ENDIAN
 #define SNDRV_PCM_FMTBIT_S16   SNDRV_PCM_FMTBIT_S16_BE
@@ -213,6 +219,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_BE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_BE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE
+#define SNDRV_PCM_FMTBIT_S20   SNDRV_PCM_FMTBIT_S20_BE
+#define SNDRV_PCM_FMTBIT_U20   SNDRV_PCM_FMTBIT_U20_BE
 #endif
 
 struct snd_pcm_file {
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 58acd00cae19..d970879944fc 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -102,6 +102,8 @@ struct snd_compr_stream;
   SNDRV_PCM_FMTBIT_S16_BE |\
   SNDRV_PCM_FMTBIT_S20_3LE |\
   SNDRV_PCM_FMTBIT_S20_3BE |\
+  SNDRV_PCM_FMTBIT_S20_LE |\
+  SNDRV_PCM_FMTBIT_S20_BE |\
   SNDRV_PCM_FMTBIT_S24_3LE |\
   SNDRV_PCM_FMTBIT_S24_3BE |\
SNDRV_PCM_FMTBIT_S32_LE |\
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..7385024041d2 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -214,6 +214,11 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_IMA_ADPCM  ((__force snd_pcm_format_t) 22)
 #defineSNDRV_PCM_FORMAT_MPEG   ((__force snd_pcm_format_t) 23)
 #defineSNDRV_PCM_FORMAT_GSM((__force snd_pcm_format_t) 24)
+#defineSNDRV_PCM_FORMAT_S20_LE ((__force snd_pcm_format_t) 25) /* \
*/
+#defineSNDRV_PCM_FORMAT_S20_BE ((__force snd_pcm_format_t) 26) /* |
*/
+#defineSNDRV_PCM_FORMAT_U20_LE ((__force snd_pcm_format_t) 27) /* | in 
four bytes, */
+#defineSNDRV_PCM_FORMAT_U20_BE ((__force snd_pcm_format_t) 28) /* / 
LSB justified  */
+/* gap in the numbering for a future standard linear format */
 #defineSNDRV_PCM_FORMAT_SPECIAL((__force snd_pcm_format_t) 31)
 #defineSNDRV_PCM_FORMAT_S24_3LE((__force snd_pcm_format_t) 32) 
/* in three bytes */
 #defineSNDRV_PCM_FORMAT_S24_3BE((__force snd_pcm_format_t) 33) 
/* in three bytes */
@@ -248,6 +253,8 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_FLOAT  SNDRV_PCM_FORMAT_FLOAT_LE
 #defineSNDRV_PCM_FORMAT_FLOAT64SNDRV_PCM_FORMAT_FLOAT64_LE
 #defineSNDRV_PCM_FORMAT_IEC958_SUBFRAME 
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE
+#defineSNDR

[PATCH v2 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

When testing AC'97 capture on UDOO board (currently the only user of
fsl_ssi driver in the AC'97 mode) it become obvious that there is a massive
distortion above certain, small input signal.

This problem has been traced to silicon errata ERR003778:
"In AC97, 16-bit mode, received data is shifted by 4-bit locations" that
has "No fix scheduled".
This errata suggests a workaround of doing a 4-bit shift back in SDMA
script for this specific operation mode, however our SDMA scripts are
shared between various SoC peripherals so we can't really modify them.

There is a simple way to avoid this problem, however, that is to disallow
recording in 16-bit mode and only support it in AC'97-native 20-bit mode.
We have to use a 4-byte format for this since SSI FIFOs do not allow 3-byte
accesses (and these aren't supported by imx-sdma driver anyway).
With this change the capture distortion is gone.

We can also add this format as an additional one supported for playback,
using this opportunity to make sure that we use CPU-endian-native formats
in AC'97 mode as we already do in I2S mode.

There is no problem in using different bit widths in playback and capture
in AC'97 mode so allow this, too.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Adapt format name to changes in the first patch from
this series.

 sound/soc/fsl/fsl_ssi.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 20ef09e1a395..c350117c8e31 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1278,14 +1278,15 @@ static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_8000_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   .formats = SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S20,
},
.capture = {
.stream_name = "AC97 Capture",
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   /* 16-bit capture is broken (errata ERR003778) */
+   .formats = SNDRV_PCM_FMTBIT_S20,
},
.ops = _ssi_dai_ops,
 };
@@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
-   if (!fsl_ssi_is_ac97(ssi_private))
+   if (!fsl_ssi_is_ac97(ssi_private)) {
ssi_private->cpu_dai_drv.symmetric_rates = 1;
+   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
+   }
 
ssi_private->cpu_dai_drv.symmetric_channels = 1;
-   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
}
 
/* Determine the FIFO depth. */

[PATCH v2 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20

[Maciej S. Szmigiero]

This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte (more conventional)
format is needed for it.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Drop "_4" suffix from these formats since they aren't
non-standard ones, use empty format slots starting from format number 25
for them, add information that they are LSB justified formats.

Corresponding alsa-lib changes will be posted as soon as this patch is
merged on the kernel side, to keep alsa-lib and kernel synchronized.

 include/sound/pcm.h |  8 
 include/sound/soc-dai.h |  2 ++
 include/uapi/sound/asound.h |  9 +
 sound/core/pcm_misc.c   | 16 
 4 files changed, 35 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 24febf9e177c..e054c583d3b3 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -169,6 +169,10 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_IMA_ADPCM _SNDRV_PCM_FMTBIT(IMA_ADPCM)
 #define SNDRV_PCM_FMTBIT_MPEG  _SNDRV_PCM_FMTBIT(MPEG)
 #define SNDRV_PCM_FMTBIT_GSM   _SNDRV_PCM_FMTBIT(GSM)
+#define SNDRV_PCM_FMTBIT_S20_LE_SNDRV_PCM_FMTBIT(S20_LE)
+#define SNDRV_PCM_FMTBIT_U20_LE_SNDRV_PCM_FMTBIT(U20_LE)
+#define SNDRV_PCM_FMTBIT_S20_BE_SNDRV_PCM_FMTBIT(S20_BE)
+#define SNDRV_PCM_FMTBIT_U20_BE_SNDRV_PCM_FMTBIT(U20_BE)
 #define SNDRV_PCM_FMTBIT_SPECIAL   _SNDRV_PCM_FMTBIT(SPECIAL)
 #define SNDRV_PCM_FMTBIT_S24_3LE   _SNDRV_PCM_FMTBIT(S24_3LE)
 #define SNDRV_PCM_FMTBIT_U24_3LE   _SNDRV_PCM_FMTBIT(U24_3LE)
@@ -202,6 +206,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_LE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_LE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE
+#define SNDRV_PCM_FMTBIT_S20   SNDRV_PCM_FMTBIT_S20_LE
+#define SNDRV_PCM_FMTBIT_U20   SNDRV_PCM_FMTBIT_U20_LE
 #endif
 #ifdef SNDRV_BIG_ENDIAN
 #define SNDRV_PCM_FMTBIT_S16   SNDRV_PCM_FMTBIT_S16_BE
@@ -213,6 +219,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_BE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_BE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE
+#define SNDRV_PCM_FMTBIT_S20   SNDRV_PCM_FMTBIT_S20_BE
+#define SNDRV_PCM_FMTBIT_U20   SNDRV_PCM_FMTBIT_U20_BE
 #endif
 
 struct snd_pcm_file {
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 58acd00cae19..d970879944fc 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -102,6 +102,8 @@ struct snd_compr_stream;
   SNDRV_PCM_FMTBIT_S16_BE |\
   SNDRV_PCM_FMTBIT_S20_3LE |\
   SNDRV_PCM_FMTBIT_S20_3BE |\
+  SNDRV_PCM_FMTBIT_S20_LE |\
+  SNDRV_PCM_FMTBIT_S20_BE |\
   SNDRV_PCM_FMTBIT_S24_3LE |\
   SNDRV_PCM_FMTBIT_S24_3BE |\
SNDRV_PCM_FMTBIT_S32_LE |\
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..7385024041d2 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -214,6 +214,11 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_IMA_ADPCM  ((__force snd_pcm_format_t) 22)
 #defineSNDRV_PCM_FORMAT_MPEG   ((__force snd_pcm_format_t) 23)
 #defineSNDRV_PCM_FORMAT_GSM((__force snd_pcm_format_t) 24)
+#defineSNDRV_PCM_FORMAT_S20_LE ((__force snd_pcm_format_t) 25) /* \
*/
+#defineSNDRV_PCM_FORMAT_S20_BE ((__force snd_pcm_format_t) 26) /* |
*/
+#defineSNDRV_PCM_FORMAT_U20_LE ((__force snd_pcm_format_t) 27) /* | in 
four bytes, */
+#defineSNDRV_PCM_FORMAT_U20_BE ((__force snd_pcm_format_t) 28) /* / 
LSB justified  */
+/* gap in the numbering for a future standard linear format */
 #defineSNDRV_PCM_FORMAT_SPECIAL((__force snd_pcm_format_t) 31)
 #defineSNDRV_PCM_FORMAT_S24_3LE((__force snd_pcm_format_t) 32) 
/* in three bytes */
 #defineSNDRV_PCM_FORMAT_S24_3BE((__force snd_pcm_format_t) 33) 
/* in three bytes */
@@ -248,6 +253,8 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_FLOAT  SNDRV_PCM_FORMAT_FLOAT_LE
 #defineSNDRV_PCM_FORMAT_FLOAT64SNDRV_PCM_FORMAT_FLOAT64_LE
 #defineSNDRV_PCM_FORMAT_IEC958_SUBFRAME 
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE
+#defineSNDRV_PCM_FORMAT_S20SNDRV_PCM_FORMAT_S20_LE
+#defineSNDRV_PCM_FORMAT_U20SNDRV_PCM_FORMAT_U20_LE
 #endif
 #ifdef SNDRV_BIG

Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4

[Maciej S. Szmigiero]

On 23.11.2017 08:40, Takashi Sakamoto wrote:
> On Nov 23 2017 08:44, Maciej S. Szmigiero wrote:
>> On 23.11.2017 00:27, Takashi Sakamoto wrote:
>>> On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
>> (..)
>>>> --- a/include/uapi/sound/asound.h
>>>> +++ b/include/uapi/sound/asound.h
>>>> @@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
>>>>    #define    SNDRV_PCM_FORMAT_DSD_U32_LE    ((__force snd_pcm_format_t) 
>>>> 50) /* DSD, 4-byte samples DSD (x32), little endian */
>>>>    #define    SNDRV_PCM_FORMAT_DSD_U16_BE    ((__force snd_pcm_format_t) 
>>>> 51) /* DSD, 2-byte samples DSD (x16), big endian */
>>>>    #define    SNDRV_PCM_FORMAT_DSD_U32_BE    ((__force snd_pcm_format_t) 
>>>> 52) /* DSD, 4-byte samples DSD (x32), big endian */
>>>> -#define    SNDRV_PCM_FORMAT_LAST    SNDRV_PCM_FORMAT_DSD_U32_BE
>>>> +#define    SNDRV_PCM_FORMAT_S20_4LE    ((__force snd_pcm_format_t) 53)    
>>>> /* in four bytes */
>>>> +#define    SNDRV_PCM_FORMAT_S20_4BE    ((__force snd_pcm_format_t) 54)    
>>>> /* in four bytes */
>>>> +#define    SNDRV_PCM_FORMAT_U20_4LE    ((__force snd_pcm_format_t) 55)    
>>>> /* in four bytes */
>>>> +#define    SNDRV_PCM_FORMAT_U20_4BE    ((__force snd_pcm_format_t) 56)    
>>>> /* in four bytes */
>>>> +#define    SNDRV_PCM_FORMAT_LAST    SNDRV_PCM_FORMAT_U20_4BE
>>>
>>> In my opinion, for this type of definition, it's better to declare 
>>> left/right-adjusted or padding side. (Of course, silence definition is 
>>> already a hint, however the lack of information forces developers to have a 
>>> careful behaviour to handle entries on the list.
>>> (I note that in current ALSA PCM interface there's no way to deliver 
>>> MSB/LSB-first information about sample format.)
>>
>> No other sound format includes this information in its name
> 
> You overlook comments in 'SNDRV_PCM_FORMAT_[U|S]24_[LE|BE]'. Let me refer to 
> them [1]:
> 
> 198 #define SNDRV_PCM_FORMAT_S24_LE ((__force snd_pcm_format_t) 6) /* low 
> three bytes */
> 199 #define SNDRV_PCM_FORMAT_S24_BE ((__force snd_pcm_format_t) 7) /* low 
> three bytes */
> 200 #define SNDRV_PCM_FORMAT_U24_LE ((__force snd_pcm_format_t) 8) /* low 
> three bytes */
> 201 #define SNDRV_PCM_FORMAT_U24_BE ((__force snd_pcm_format_t) 9) /* low 
> three bytes */

Yes, I can add this information in a comment, just like these formats are
described as "low three bytes" (in other words, LSB justified formats)

> In your way, these types of format can be represented by 
> 'SNDRV_PCM_FORMAT_[U|S]24_4[LE|BE]', thus for playback direction they
> mean:
> 
> ```
> #include 
> #include 
> 
> uint32_t *buf;
> uint32_t sample;
> snd_pcm_format_t format;
> 
> sample = generate_a_sample();
> (sample & ~0x00ff) /* invalid bits as sample */
> 
> if (format == SNDRV_PCM_FORMAT_[U|S]24_LE) {
>   buf[0] = htole32(sample);
> else
>   buf[0] = htobe32(sample);
> 
> /* transfer content of the buf via ALSA kernel stuffs. */
> ```
> 
> The comments are good enough for application developers in an aspect of a 
> position for padding.
> 
> In general, studying from the past is preferable behaviour to be genius, 
> however accumulated history includes mistakes and defects. Just pretending 
> the past is not so genius, without further consideration.
> 
> Actually additions of the rest of entries for PCM format were done without 
> enough cares of what information they give to application developers. Adding 
> new entries is easier than fixing and improving them once exposed. It's a 
> reason that they're left what they're.
> 
> I wish you had enough care to assist applications developers. Without 
> applications, drivers are worthless and just waste of code base.

Right, I will add this information to a comment.

(..)
>>> Additionally, alsa-lib includes some codes related to the definition[1]. If 
>>> you'd like to thing goes well out of ALSA SoC part, it's better to submit 
>>> changes to the library as well.
>>>
>>> [1] 
>>> http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD
>>
>> I have alsa-lib changes ready for these formats - they were needed to
>> test these patches, will post them when this is merged on the kernel
>> side (in case some changes are needed which affect both).
> 
> Please pay enough care when writing patch comment. Silence means nothing, at 
> least for reviewers, even if you have good preparations.
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h?h=sound-4.15-rc1#n198

I will add this information (about alsa-lib changes) to commit notes in this 
patch description.

> Regards
> 
> Takashi Sakamoto

Best regards,
Maciej Szmigiero

Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20_4

[Maciej S. Szmigiero]

On 23.11.2017 09:08, Takashi Iwai wrote:
> On Wed, 22 Nov 2017 20:17:34 +0100,
>  Maciej S. Szmigiero  wrote:
>>
>> This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
>> 20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
>> allow 3-byte accesses (including DMA) so a 4-byte format is needed for it.
>>
>> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
>> ---
>>  include/sound/pcm.h |  8 
>>  include/sound/soc-dai.h |  2 ++
>>  include/uapi/sound/asound.h | 10 +-
>>  sound/core/pcm_misc.c   | 16 
>>  4 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index 24febf9e177c..7ad2d3f0934f 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -191,6 +191,10 @@ struct snd_pcm_ops {
>>  #define SNDRV_PCM_FMTBIT_DSD_U32_LE _SNDRV_PCM_FMTBIT(DSD_U32_LE)
>>  #define SNDRV_PCM_FMTBIT_DSD_U16_BE _SNDRV_PCM_FMTBIT(DSD_U16_BE)
>>  #define SNDRV_PCM_FMTBIT_DSD_U32_BE _SNDRV_PCM_FMTBIT(DSD_U32_BE)
>> +#define SNDRV_PCM_FMTBIT_S20_4LE_SNDRV_PCM_FMTBIT(S20_4LE)
>> +#define SNDRV_PCM_FMTBIT_U20_4LE_SNDRV_PCM_FMTBIT(U20_4LE)
>> +#define SNDRV_PCM_FMTBIT_S20_4BE_SNDRV_PCM_FMTBIT(S20_4BE)
>> +#define SNDRV_PCM_FMTBIT_U20_4BE_SNDRV_PCM_FMTBIT(U20_4BE)
> 
> The conventional names aren't with "4" suffix,
> e.g. SNDRV_PCM_FMTBIT_S20_LE.

Will rename it in a respin.
> Also, there are still empty slots under 32, e.g. start from 25.
> The formats over 31 were used for 3 bytes or other unusual formats
> (although nowadays it makes little sense), and the slots < 32 would
> fit for 4 bytes linear format.

Will renumber these formats to start from 25 in a respin.

> It's still an open question whether we should increase the protocol
> number when we add a new PCM format definition.  I guess it's not, as
> the ABI behavior itself doesn't change, but I might have overlooked
> some possible breakage.

This isn't an incompatible, application-breaking, ABI change (it is
rather kind of an additional "ABI"), so I think an ALSA protocol number
should not be increased for it.

> thanks,
> 
> Takashi
> 

Thanks,
Maciej

Re: [PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S, U}20_4

[Maciej S. Szmigiero]

On 23.11.2017 00:27, Takashi Sakamoto wrote:
> On Nov 23 2017 04:17, Maciej S. Szmigiero wrote:
(..)
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
>>   #define    SNDRV_PCM_FORMAT_DSD_U32_LE    ((__force snd_pcm_format_t) 50) 
>> /* DSD, 4-byte samples DSD (x32), little endian */
>>   #define    SNDRV_PCM_FORMAT_DSD_U16_BE    ((__force snd_pcm_format_t) 51) 
>> /* DSD, 2-byte samples DSD (x16), big endian */
>>   #define    SNDRV_PCM_FORMAT_DSD_U32_BE    ((__force snd_pcm_format_t) 52) 
>> /* DSD, 4-byte samples DSD (x32), big endian */
>> -#define    SNDRV_PCM_FORMAT_LAST    SNDRV_PCM_FORMAT_DSD_U32_BE
>> +#define    SNDRV_PCM_FORMAT_S20_4LE    ((__force snd_pcm_format_t) 53)    
>> /* in four bytes */
>> +#define    SNDRV_PCM_FORMAT_S20_4BE    ((__force snd_pcm_format_t) 54)    
>> /* in four bytes */
>> +#define    SNDRV_PCM_FORMAT_U20_4LE    ((__force snd_pcm_format_t) 55)    
>> /* in four bytes */
>> +#define    SNDRV_PCM_FORMAT_U20_4BE    ((__force snd_pcm_format_t) 56)    
>> /* in four bytes */
>> +#define    SNDRV_PCM_FORMAT_LAST    SNDRV_PCM_FORMAT_U20_4BE
> 
> In my opinion, for this type of definition, it's better to declare 
> left/right-adjusted or padding side. (Of course, silence definition is 
> already a hint, however the lack of information forces developers to have a 
> careful behaviour to handle entries on the list.> (I note that in current 
> ALSA PCM interface there's no way to deliver MSB/LSB-first information about 
> sample format.)

No other sound format includes this information in its name so if we name
these formats SNDRV_PCM_FORMAT_{S, U}20LSB_4 they are going to have it
inconsistent with every other one (I assume you meant to include such
information in a format name?).

But information about whether this format is MSB or LSB justified can be
added in a comment so the situation is clear for other developers from
the definition without needing to read the actual processing code.

> Additionally, alsa-lib includes some codes related to the definition[1]. If 
> you'd like to thing goes well out of ALSA SoC part, it's better to submit 
> changes to the library as well.
> 
> [1] 
> http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm_misc.c;h=5420b1895713a3aec3624a5218794a7b49baf167;hb=HEAD

I have alsa-lib changes ready for these formats - they were needed to
test these patches, will post them when this is merged on the kernel
side (in case some changes are needed which affect both).

> Regards
> 
> Takashi Sakamoto

Best regards,
Maciej Szmigiero

[PATCH 2/2] ASoC: fsl_ssi: add 20-bit sample format for AC'97 and use it for capture

[Maciej S. Szmigiero]

When testing AC'97 capture on UDOO board (currently the only user of
fsl_ssi driver in the AC'97 mode) it become obvious that there is a massive
distortion above certain, small input signal.

This problem has been traced to silicon errata ERR003778:
"In AC97, 16-bit mode, received data is shifted by 4-bit locations" that
has "No fix scheduled".
This errata suggests a workaround of doing a 4-bit shift back in SDMA
script for this specific operation mode, however our SDMA scripts are
shared between various SoC peripherals so we can't really modify them.

There is a simple way to avoid this problem, however, that is to disallow
recording in 16-bit mode and only support it in AC'97-native 20-bit mode.
We have to use a 4-byte format for this since SSI FIFOs do not allow 3-byte
accesses (and these aren't supported by imx-sdma driver anyway).
With this change the capture distortion is gone.

We can also add this format as an additional one supported for playback,
using this opportunity to make sure that we use CPU-endian-native formats
in AC'97 mode as we already do in I2S mode.

There is no problem in using different bit widths in playback and capture
in AC'97 mode so allow this, too.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 20ef09e1a395..2aed089fdd76 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1278,14 +1278,15 @@ static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_8000_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   .formats = SNDRV_PCM_FMTBIT_S16 | SNDRV_PCM_FMTBIT_S20_4,
},
.capture = {
.stream_name = "AC97 Capture",
.channels_min = 2,
.channels_max = 2,
.rates = SNDRV_PCM_RATE_48000,
-   .formats = SNDRV_PCM_FMTBIT_S16_LE,
+   /* 16-bit capture is broken (errata ERR003778) */
+   .formats = SNDRV_PCM_FMTBIT_S20_4,
},
.ops = _ssi_dai_ops,
 };
@@ -1557,11 +1558,12 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, "fsl,ssi-asynchronous", NULL)) {
-   if (!fsl_ssi_is_ac97(ssi_private))
+   if (!fsl_ssi_is_ac97(ssi_private)) {
ssi_private->cpu_dai_drv.symmetric_rates = 1;
+   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
+   }
 
ssi_private->cpu_dai_drv.symmetric_channels = 1;
-   ssi_private->cpu_dai_drv.symmetric_samplebits = 1;
}
 
/* Determine the FIFO depth. */

[PATCH 1/2] ALSA: pcm: add SNDRV_PCM_FORMAT_{S,U}20_4

[Maciej S. Szmigiero]

This format is similar to existing SNDRV_PCM_FORMAT_{S,U}20_3 that keep
20-bit PCM samples in 3 bytes, however i.MX6 platform SSI FIFO does not
allow 3-byte accesses (including DMA) so a 4-byte format is needed for it.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 include/sound/pcm.h |  8 
 include/sound/soc-dai.h |  2 ++
 include/uapi/sound/asound.h | 10 +-
 sound/core/pcm_misc.c   | 16 
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 24febf9e177c..7ad2d3f0934f 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -191,6 +191,10 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_DSD_U32_LE_SNDRV_PCM_FMTBIT(DSD_U32_LE)
 #define SNDRV_PCM_FMTBIT_DSD_U16_BE_SNDRV_PCM_FMTBIT(DSD_U16_BE)
 #define SNDRV_PCM_FMTBIT_DSD_U32_BE_SNDRV_PCM_FMTBIT(DSD_U32_BE)
+#define SNDRV_PCM_FMTBIT_S20_4LE   _SNDRV_PCM_FMTBIT(S20_4LE)
+#define SNDRV_PCM_FMTBIT_U20_4LE   _SNDRV_PCM_FMTBIT(U20_4LE)
+#define SNDRV_PCM_FMTBIT_S20_4BE   _SNDRV_PCM_FMTBIT(S20_4BE)
+#define SNDRV_PCM_FMTBIT_U20_4BE   _SNDRV_PCM_FMTBIT(U20_4BE)
 
 #ifdef SNDRV_LITTLE_ENDIAN
 #define SNDRV_PCM_FMTBIT_S16   SNDRV_PCM_FMTBIT_S16_LE
@@ -202,6 +206,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_LE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_LE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE
+#define SNDRV_PCM_FMTBIT_S20_4 SNDRV_PCM_FMTBIT_S20_4LE
+#define SNDRV_PCM_FMTBIT_U20_4 SNDRV_PCM_FMTBIT_U20_4LE
 #endif
 #ifdef SNDRV_BIG_ENDIAN
 #define SNDRV_PCM_FMTBIT_S16   SNDRV_PCM_FMTBIT_S16_BE
@@ -213,6 +219,8 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_FLOAT SNDRV_PCM_FMTBIT_FLOAT_BE
 #define SNDRV_PCM_FMTBIT_FLOAT64   SNDRV_PCM_FMTBIT_FLOAT64_BE
 #define SNDRV_PCM_FMTBIT_IEC958_SUBFRAME SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE
+#define SNDRV_PCM_FMTBIT_S20_4 SNDRV_PCM_FMTBIT_S20_4BE
+#define SNDRV_PCM_FMTBIT_U20_4 SNDRV_PCM_FMTBIT_U20_4BE
 #endif
 
 struct snd_pcm_file {
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 58acd00cae19..16aec0cc96f5 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -102,6 +102,8 @@ struct snd_compr_stream;
   SNDRV_PCM_FMTBIT_S16_BE |\
   SNDRV_PCM_FMTBIT_S20_3LE |\
   SNDRV_PCM_FMTBIT_S20_3BE |\
+  SNDRV_PCM_FMTBIT_S20_4LE |\
+  SNDRV_PCM_FMTBIT_S20_4BE |\
   SNDRV_PCM_FMTBIT_S24_3LE |\
   SNDRV_PCM_FMTBIT_S24_3BE |\
SNDRV_PCM_FMTBIT_S32_LE |\
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..69b661816491 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -236,7 +236,11 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_DSD_U32_LE ((__force snd_pcm_format_t) 50) 
/* DSD, 4-byte samples DSD (x32), little endian */
 #defineSNDRV_PCM_FORMAT_DSD_U16_BE ((__force snd_pcm_format_t) 51) 
/* DSD, 2-byte samples DSD (x16), big endian */
 #defineSNDRV_PCM_FORMAT_DSD_U32_BE ((__force snd_pcm_format_t) 52) 
/* DSD, 4-byte samples DSD (x32), big endian */
-#defineSNDRV_PCM_FORMAT_LAST   SNDRV_PCM_FORMAT_DSD_U32_BE
+#defineSNDRV_PCM_FORMAT_S20_4LE((__force snd_pcm_format_t) 53) 
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_S20_4BE((__force snd_pcm_format_t) 54) 
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_U20_4LE((__force snd_pcm_format_t) 55) 
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_U20_4BE((__force snd_pcm_format_t) 56) 
/* in four bytes */
+#defineSNDRV_PCM_FORMAT_LAST   SNDRV_PCM_FORMAT_U20_4BE
 
 #ifdef SNDRV_LITTLE_ENDIAN
 #defineSNDRV_PCM_FORMAT_S16SNDRV_PCM_FORMAT_S16_LE
@@ -248,6 +252,8 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_FLOAT  SNDRV_PCM_FORMAT_FLOAT_LE
 #defineSNDRV_PCM_FORMAT_FLOAT64SNDRV_PCM_FORMAT_FLOAT64_LE
 #defineSNDRV_PCM_FORMAT_IEC958_SUBFRAME 
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE
+#defineSNDRV_PCM_FORMAT_S20_4  SNDRV_PCM_FORMAT_S20_4LE
+#defineSNDRV_PCM_FORMAT_U20_4  SNDRV_PCM_FORMAT_U20_4LE
 #endif
 #ifdef SNDRV_BIG_ENDIAN
 #defineSNDRV_PCM_FORMAT_S16SNDRV_PCM_FORMAT_S16_BE
@@ -259,6 +265,8 @@ typedef int __bitwise snd_pcm_format_t;
 #defineSNDRV_PCM_FORMAT_FLOAT  SNDRV_PCM_FORMAT_FLOAT_BE
 #defineSNDRV_PCM_FORMAT_FLOAT64SNDRV_PCM_FORMAT_FLOAT64_BE
 #defineSNDRV_PCM_FORMAT_IEC958_SUBFRAME 
SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE
+#

[PATCH v2 2/2] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

In AC'97 mode we configure and start SSI RX / TX on probe path via
a call to _fsl_ssi_set_dai_fmt() function.
We don't need to call this function again later and in fact don't want to
do it since this function temporarily sets STCR, SRCR and SCR to some
intermediate values.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: The SACCST setup code was split out into a separate
commit.

 sound/soc/fsl/fsl_ssi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 375aaaf6080d..70df6bf832df 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1116,6 +1116,9 @@ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai 
*cpu_dai, unsigned int fmt)
 {
struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
 
+   if (fsl_ssi_is_ac97(ssi_private))
+   return 0;
+
return _fsl_ssi_set_dai_fmt(cpu_dai->dev, ssi_private, fmt);
 }
 

[PATCH v2 1/2] ASoC: fsl_ssi: only enable proper channel slots in AC'97 mode

[Maciej S. Szmigiero]

We need to make sure that only proper channel slots (in SACCST register)
are enabled at playback start time since some AC'97 CODECs (like VT1613 on
UDOO board) were observed requesting via SLOTREQ spurious ones just after
an AC'97 link is started but before the CODEC is configured by its driver.
When a bit for some channel slot is set in a SLOTREQ request then SSI sets
the relevant bit in SACCST automatically, which then 'sticks' until it is
manually unset.
The SACCST register is not writable directly, we have to use SACCDIS and
SACCEN registers to configure it instead (these aren't normal registers:
writing a '1' bit at some position in SACCEN sets the relevant bit in
SACCST; SACCDIS operates in a similar way but allows unsetting bits in
SACCST).

Theoretically, this should be necessary only for the very first playback
but since some CODECs are so untrustworthy and extra channel slots enabled
mean ruined playback let's play safe here and make sure that no extra
slots are enabled in SACCST every time a playback is started.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
Changes from v1: Split out this part from
"fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode" commit,
describe the problem and its solution better both in the commit message and
in the code, move the SACCST setup code into a separate function and call
it from TX config instead of doing it from trigger handler function.

 sound/soc/fsl/fsl_ssi.c | 52 +++--
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 48bb850a34d9..375aaaf6080d 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -574,8 +574,54 @@ static void fsl_ssi_rx_config(struct fsl_ssi_private 
*ssi_private, bool enable)
fsl_ssi_config(ssi_private, enable, _private->rxtx_reg_val.rx);
 }
 
+static void fsl_ssi_tx_ac97_saccst_setup(struct fsl_ssi_private *ssi_private)
+{
+   struct regmap *regs = ssi_private->regs;
+
+   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+   if (!ssi_private->soc->imx21regs) {
+   /*
+* Note that these below aren't just normal registers.
+* They are a way to disable or enable bits in SACCST
+* register:
+* - writing a '1' bit at some position in SACCEN sets the
+* relevant bit in SACCST,
+* - writing a '1' bit at some position in SACCDIS unsets
+* the relevant bit in SACCST register.
+*
+* The two writes below first disable all channels slots,
+* then enable just slots 3 & 4 ("PCM Playback Left Channel"
+* and "PCM Playback Right Channel").
+*/
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
+}
+
 static void fsl_ssi_tx_config(struct fsl_ssi_private *ssi_private, bool enable)
 {
+   /*
+* Why are we setting up SACCST everytime we are starting a
+* playback?
+* Some CODECs (like VT1613 CODEC on UDOO board) like to
+* (sometimes) set extra bits in their SLOTREQ requests.
+* When a bit is set in a SLOTREQ request then SSI sets the
+* relevant bit in SACCST automatically (it is enough if a bit was
+* set in a SLOTREQ just once, bits in SACCST are 'sticky').
+* If an extra slot gets enabled that's a disaster for playback
+* because some of normal left or right channel samples are
+* redirected instead to this extra slot.
+*
+* A workaround implemented in fsl-asoc-card of setting an
+* appropriate CODEC register so that slots 3 & 4 (the normal
+* stereo playback slots) are used for S/PDIF seems to mostly fix
+* this issue on the UDOO board but since this CODEC is so
+* untrustworthy let's play safe here and make sure that no extra
+* slots are enabled every time a playback is started.
+*/
+   if (enable && fsl_ssi_is_ac97(ssi_private))
+   fsl_ssi_tx_ac97_saccst_setup(ssi_private);
+
fsl_ssi_config(ssi_private, enable, _private->rxtx_reg_val.tx);
 }
 
@@ -630,12 +676,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
 
-   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
-   if (!ssi_private->soc->imx21regs) {
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
-   }
-
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
 * codec before a stream is started.

Re: [alsa-devel] [PATCH 2/2] ASoC: fsl_ssi: serialize AC'97 register access operations

[Maciej S. Szmigiero]

On 21.11.2017 02:52, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:16:07PM +0100, Maciej S. Szmigiero wrote:
(..)
>>  static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
>> @@ -1287,16 +1295,18 @@ static unsigned short fsl_ssi_ac97_read(struct 
>> snd_ac97 *ac97,
>>  {
>>  struct regmap *regs = fsl_ac97_data->regs;
>>  
>> -unsigned short val = -1;
>> +unsigned short val = 0;
>>  u32 reg_val;
>>  unsigned int lreg;
>>  int ret;
>>  
>> +mutex_lock(_ac97_data->ac97_reg_lock);
>> +
>>  ret = clk_prepare_enable(fsl_ac97_data->clk);
>>  if (ret) {
>>  pr_err("ac97 read clk_prepare_enable failed: %d\n",
>>  ret);
>> -return -1;
>> +goto ret_unlock;
> 
> It will return val (== 0) in this case. Will this be correctly
> handled by callers? I find sound/ac97/bus.c checks if ret < 0
> for ops->read().

Both 0 and -1 (0x really) are valid register values.

AC'97 code that is used in companion with fsl_ssi lives in sound/pci/ac97
directory and does register reads via snd_ac97_read() function in
ac97_codec.c file (located in that directory).
This function has no such check.

The reason why AC'97 specs call for unknown register read to return zero
is that if there is some capability bit in a register then this bit is
set when a CODEC has such capability.
If we return -1 then it means that in this case a CODEC would be
detected as having all possible capabilities that are exposed via this
register (including these that aren't really supported) while if we
return 0 instead then we merely wouldn't make use of some that are actually
present.

> 
> So it might be better to add "val = ret;" before goto? Or use
> val instead of ret directly?

Then we would be returning an error number from clk_prepare_enable()
as a register value which would be almost certainly wrong.

Note that this method returns unsigned short - a type with the same width
as an AC'97 register, so there are no unused values in this type which
could be reliably used to signify that an error had happened.

Maciej

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

On 21.11.2017 02:14, Nicolin Chen wrote:
> On Tue, Nov 21, 2017 at 01:32:09AM +0100, Maciej S. Szmigiero wrote:
>> On 21.11.2017 01:00, Nicolin Chen wrote:
>>> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
>> (..)
>>>> We need to make sure, however, that only proper channel slots are enabled
>>>> at playback start time since some AC'97 CODECs (like VT1613) were observed
>>>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
>>>> an AC'97 link is started but before the CODEC is configured by its driver.
>>>
>>> I don't really understand this part. Why do we need to *make sure*
>>> and set SACCDIS and SACCEN again since they're initialized already> 
>>> Could you please elaborate a bit more?
>>
>> SACCDIS and SACCEN aren't just normal registers.
>> They are a way to disable or enable bits in SACCST register - writing
>> a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
>> writing a '1' bit at some position in SACCEN sets the relevant bit in
>> SACCST.
>>
>> But a bit in SACCST can also be set (and so an AC'97 channel slot
>> enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
>> enough if this bit was set in SLOTREQ just once, bits in SACCST are
>> 'sticky').
> 
> This makes sense now. It could be mentioned a bit in the commit log
> as well.

Will do.

>> If an extra slot gets enabled that's a disaster for playback because some
>> of normal left or right channel samples are instead redirected to this
>> extra slot.
>>
>> Unfortunately, the VT1613 codec on UDOO board (which is the only user of
>> fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
>> set extra bits in its SLOTREQ request - I've confirmed this on two boards
>> bought a few months apart directly from UDOO shop.
>>
>> A workaround implemented in fsl-asoc-card of setting an appropriate
>> CODEC register so that slots 3/4 (the normal stereo playback slots) are
>> used for S/PDIF seems to mostly fix this issue but since this codec is so
>> untrustworthy then:
> 
> If this move is also a workaround, probably it'd be better to have
> an fsl_ssi_ac97_xxx_war() function with some comments/descriptions,
> and include it in the config(). Since it doesn't hurt to set these
> registers again, I am personally fine with that. But it needs to be
> clear -- otherwise, people may try to remove it later with a change
> like: Removing redundant SACCEN/SACCDIS settings since they're done
> during probe().
> 

That's a good point, will do.

Maciej

Re: [alsa-devel] [PATCH 1/2] ASoC: fsl_ssi: AC'97 ops need regmap, clock and cleaning up on failure

[Maciej S. Szmigiero]

On 21.11.2017 01:32, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:14:55PM +0100, Maciej S. Szmigiero wrote:
(..)
>> @@ -1460,12 +1460,6 @@ static int fsl_ssi_probe(struct platform_device *pdev)
>>  sizeof(fsl_ssi_ac97_dai));
>>  
>>  fsl_ac97_data = ssi_private;
> 
> By the way, is there any better way to register the ops for AC97
> while we could pass the ssi_private so as to remove the global
> fsl_ac97_data?

This might be possible (if SSI private data is provided to AC'97 codec in
codecs/ac97.c in platform device data which then is modified to make use
of it), but currently ASoC AC'97 only supports one controller per system
so for a real gain this limitation would have to be addressed first.

Maciej

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

On 21.11.2017 01:00, Nicolin Chen wrote:
> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote:
(..)
>> We need to make sure, however, that only proper channel slots are enabled
>> at playback start time since some AC'97 CODECs (like VT1613) were observed
>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
>> an AC'97 link is started but before the CODEC is configured by its driver.
> 
> I don't really understand this part. Why do we need to *make sure*
> and set SACCDIS and SACCEN again since they're initialized already> 
> Could you please elaborate a bit more?

SACCDIS and SACCEN aren't just normal registers.
They are a way to disable or enable bits in SACCST register - writing
a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST,
writing a '1' bit at some position in SACCEN sets the relevant bit in
SACCST.

But a bit in SACCST can also be set (and so an AC'97 channel slot
enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is
enough if this bit was set in SLOTREQ just once, bits in SACCST are
'sticky').
If an extra slot gets enabled that's a disaster for playback because some
of normal left or right channel samples are instead redirected to this
extra slot.

Unfortunately, the VT1613 codec on UDOO board (which is the only user of
fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes)
set extra bits in its SLOTREQ request - I've confirmed this on two boards
bought a few months apart directly from UDOO shop.

A workaround implemented in fsl-asoc-card of setting an appropriate
CODEC register so that slots 3/4 (the normal stereo playback slots) are
used for S/PDIF seems to mostly fix this issue but since this codec is so
untrustworthy then:
>> let's play safe here and make sure that no extra slots are enabled
>> every time a playback is started.

(..)
>> @@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream 
>> *substream, int cmd,
>>  case SNDRV_PCM_TRIGGER_START:
>>  case SNDRV_PCM_TRIGGER_RESUME:
>>  case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> -if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> +/* no SACC{ST,EN,DIS} regs on imx21-class SSI */
>> +if (fsl_ssi_is_ac97(ssi_private) &&
>> +!ssi_private->soc->imx21regs) {
>> +regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
>> +regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
>> +}
> 
> And second, why could we ignore them for STREAM_CAPTURE here?

Capture is not affected by SACCST register content.

> Well, at least this part could be moved into fsl_ssi_tx_config()
> since we have an abstraction layer of register configurations.

Will move this into fsl_ssi_tx_config() in a respin.

> 
> Thanks
> Nic

Thanks,
Maciej

[PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode

[Maciej S. Szmigiero]

In AC'97 mode we configure and start SSI RX / TX on probe path via
a call to _fsl_ssi_set_dai_fmt() function.
We don't need to call this function again later and in fact don't want to
do it since this function temporarily sets STCR, SRCR and SCR to some
intermediate values.

We need to make sure, however, that only proper channel slots are enabled
at playback start time since some AC'97 CODECs (like VT1613) were observed
requesting via SLOTREQ (and so enabling at SSI) spurious ones just after
an AC'97 link is started but before the CODEC is configured by its driver.
Theoretically, this should be necessary only for the very first playback
but let's play safe here and make sure that no extra slots are enabled
every time a playback is started.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 48bb850a34d9..dad80b4b0cfc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -630,12 +630,6 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
 
-   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
-   if (!ssi_private->soc->imx21regs) {
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
-   }
-
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
 * codec before a stream is started.
@@ -1076,6 +1070,9 @@ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai 
*cpu_dai, unsigned int fmt)
 {
struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
 
+   if (fsl_ssi_is_ac97(ssi_private))
+   return 0;
+
return _fsl_ssi_set_dai_fmt(cpu_dai->dev, ssi_private, fmt);
 }
 
@@ -1149,9 +1146,16 @@ static int fsl_ssi_trigger(struct snd_pcm_substream 
*substream, int cmd,
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+   if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+   if (fsl_ssi_is_ac97(ssi_private) &&
+   !ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
+
fsl_ssi_tx_config(ssi_private, true);
-   else
+   } else
fsl_ssi_rx_config(ssi_private, true);
break;
 

[PATCH] ASoC: fsl_ssi: remove duplicated flag setting in fsl_ssi_setup_reg_vals()

[Maciej S. Szmigiero]

We don't need to set CCSR_SSI_SIER_RFF0_EN / CCSR_SSI_SIER_TFE0_EN bits
in reg->rx.sier / reg->tx.sier variables in a non-AC'97 mode considering we
had just initialized these variables to these very values unconditionally a
few lines earlier.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index f2f51e06e22c..48bb850a34d9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -597,9 +597,7 @@ static void fsl_ssi_setup_reg_vals(struct fsl_ssi_private 
*ssi_private)
 
if (!fsl_ssi_is_ac97(ssi_private)) {
reg->rx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_RE;
-   reg->rx.sier |= CCSR_SSI_SIER_RFF0_EN;
reg->tx.scr = CCSR_SSI_SCR_SSIEN | CCSR_SSI_SCR_TE;
-   reg->tx.sier |= CCSR_SSI_SIER_TFE0_EN;
}
 
if (ssi_private->use_dma) {

[PATCH 1/2] ASoC: fsl_ssi: AC'97 ops need regmap, clock and cleaning up on failure

[Maciej S. Szmigiero]

AC'97 ops (register read / write) need SSI regmap and clock, so they have
to be set after them.

We also need to set these ops back to NULL if we fail the probe.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index dad80b4b0cfc..a71bb8391f61 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1460,12 +1460,6 @@ static int fsl_ssi_probe(struct platform_device *pdev)
sizeof(fsl_ssi_ac97_dai));
 
fsl_ac97_data = ssi_private;
-
-   ret = snd_soc_set_ac97_ops_of_reset(_ssi_ac97_ops, pdev);
-   if (ret) {
-   dev_err(>dev, "could not set AC'97 ops\n");
-   return ret;
-   }
} else {
/* Initialize this copy of the CPU DAI driver structure */
memcpy(_private->cpu_dai_drv, _ssi_dai_template,
@@ -1576,6 +1570,14 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return ret;
}
 
+   if (fsl_ssi_is_ac97(ssi_private)) {
+   ret = snd_soc_set_ac97_ops_of_reset(_ssi_ac97_ops, pdev);
+   if (ret) {
+   dev_err(>dev, "could not set AC'97 ops\n");
+   goto error_ac97_ops;
+   }
+   }
+
ret = devm_snd_soc_register_component(>dev, _ssi_component,
  _private->cpu_dai_drv, 1);
if (ret) {
@@ -1659,6 +1661,10 @@ static int fsl_ssi_probe(struct platform_device *pdev)
fsl_ssi_debugfs_remove(_private->dbg_stats);
 
 error_asoc_register:
+   if (fsl_ssi_is_ac97(ssi_private))
+   snd_soc_set_ac97_ops(NULL);
+
+error_ac97_ops:
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 

[PATCH 2/2] ASoC: fsl_ssi: serialize AC'97 register access operations

[Maciej S. Szmigiero]

AC'97 register access operations (both read and write) on SSI use a one,
shared set of SSI registers for AC'97 register address and data.
This means that only one such access is possible at a time and so all these
operations need to be serialized.

Since an AC'97 register access operation in this driver takes 100us+ let's
use a mutex for this.

Use this opportunity to also change a default value returned from AC'97
register read function from -1 to 0, since that's what AC'97 specs require
to be returned when unknown / undefined registers are read.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index a71bb8391f61..9dea1b16de82 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -38,6 +38,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -265,6 +266,8 @@ struct fsl_ssi_private {
 
u32 fifo_watermark;
u32 dma_maxburst;
+
+   struct mutex ac97_reg_lock;
 };
 
 /*
@@ -1262,11 +1265,13 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
if (reg > 0x7f)
return;
 
+   mutex_lock(_ac97_data->ac97_reg_lock);
+
ret = clk_prepare_enable(fsl_ac97_data->clk);
if (ret) {
pr_err("ac97 write clk_prepare_enable failed: %d\n",
ret);
-   return;
+   goto ret_unlock;
}
 
lreg = reg <<  12;
@@ -1280,6 +1285,9 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
udelay(100);
 
clk_disable_unprepare(fsl_ac97_data->clk);
+
+ret_unlock:
+   mutex_unlock(_ac97_data->ac97_reg_lock);
 }
 
 static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
@@ -1287,16 +1295,18 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
 {
struct regmap *regs = fsl_ac97_data->regs;
 
-   unsigned short val = -1;
+   unsigned short val = 0;
u32 reg_val;
unsigned int lreg;
int ret;
 
+   mutex_lock(_ac97_data->ac97_reg_lock);
+
ret = clk_prepare_enable(fsl_ac97_data->clk);
if (ret) {
pr_err("ac97 read clk_prepare_enable failed: %d\n",
ret);
-   return -1;
+   goto ret_unlock;
}
 
lreg = (reg & 0x7f) <<  12;
@@ -1311,6 +1321,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
 
clk_disable_unprepare(fsl_ac97_data->clk);
 
+ret_unlock:
+   mutex_unlock(_ac97_data->ac97_reg_lock);
return val;
 }
 
@@ -1571,6 +1583,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
}
 
if (fsl_ssi_is_ac97(ssi_private)) {
+   mutex_init(_private->ac97_reg_lock);
ret = snd_soc_set_ac97_ops_of_reset(_ssi_ac97_ops, pdev);
if (ret) {
dev_err(>dev, "could not set AC'97 ops\n");
@@ -1665,6 +1678,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
snd_soc_set_ac97_ops(NULL);
 
 error_ac97_ops:
+   if (fsl_ssi_is_ac97(ssi_private))
+   mutex_destroy(_private->ac97_reg_lock);
+
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
@@ -1683,8 +1699,10 @@ static int fsl_ssi_remove(struct platform_device *pdev)
if (ssi_private->soc->imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
-   if (fsl_ssi_is_ac97(ssi_private))
+   if (fsl_ssi_is_ac97(ssi_private)) {
snd_soc_set_ac97_ops(NULL);
+   mutex_destroy(_private->ac97_reg_lock);
+   }
 
return 0;
 }

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

[Maciej S. Szmigiero]

On 01.02.2016 22:10, Mark Brown wrote:
> On Mon, Feb 01, 2016 at 05:58:06PM +0100, Maciej S. Szmigiero wrote:
> 
>> Looks like a possible solution would be to change
>> regmap_raw_read() to do read using _regmap_read in
>> case the cache is bypassed and there is no ->read
>> callback defined for regmap implementation.
> 
> No, that's completely broken.  We can't do a raw read from a regmap that
> doesn't offer raw access and we shouldn't pretend to do so.  If the
> caller is capable of substituting a register by register read the caller
> should take responsibility for that.

So can regcache initialization be changed to use register by register read
in case raw read fails?

Since other option for drivers like SSI which are memory mapped and
don't offer ability to reset their register values to default would be to
explicitly write driver hardcoded defaults also by doing
register by register write on probe time.

But this would force driver to keep the defaults which I think is bad
(especially for driver that supports multiple generations of hardware
like fsl_ssi which may have different default values).

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

[Maciej S. Szmigiero]

Hi Fabio,

On 01.02.2016 13:05, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Mon, Jan 18, 2016 at 5:07 PM, Maciej S. Szmigiero
> <m...@maciej.szmigiero.name> wrote:
>> There is no guarantee that on fsl_ssi module load
>> SSI registers will have their power-on-reset values.
>>
>> In fact, if the driver is reloaded the values in
>> registers will be whatever they were set to previously.
>>
>> However, the cache needs to be fully populated at probe
>> time to avoid non-atomic allocations during register
>> access.
>>
>> Special case here is imx21-class SSI, since
>> according to datasheet it don't have SACC{ST,EN,DIS}
>> regs.
>>
>> This fixes hard lockup on fsl_ssi module reload,
>> at least in AC'97 mode.
>>
>> Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to 
>> support MEGA Fast")
>>
>> Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
> 
> I know I have already tested this and it worked fine on a mx6sabresd,
> but running linux-next 20160201 on a mx6sl-evk the ssi driver does not
> probe anymore:
> 
> [2.216954] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
> [2.223412] fsl-asoc-card sound: snd_soc_register_card failed (-517)
> [2.230258] imx-wm8962 sound: ASoC: CPU DAI 202c000.ssi not registered
> [2.236806] imx-wm8962 sound: snd_soc_register_card failed (-517)
> [2.244782] snvs_rtc 20cc000.snvs:snvs-rtc-lp: setting system clock
> to 1970-01-01 00:01:14 UTC (74)
> [2.285864] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
> [2.292335] fsl-asoc-card sound: snd_soc_register_card failed (-517)
> [2.299572] imx-wm8962 sound: ASoC: CPU DAI 202c000.ssi not registered
> [2.306121] imx-wm8962 sound: snd_soc_register_card failed (-517)
> 
> Reverting this patch fixes the problem.
> 
> Wandboard also has the same issue:
> 
> http://arm-soc.lixom.net/bootlogs/next/next-20160201/wandboard-arm-imx_v6_v7_defconfig.html
> 
> Any suggestions?
> 
> Thanks
> 

Is regmap patch from 
http://www.spinics.net/lists/kernel/msg2161934.html
applied to the tested tree?

Best regards,
Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

[Maciej S. Szmigiero]

On 01.02.2016 13:13, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
> <m...@maciej.szmigiero.name> wrote:
>> Is regmap patch from
>> http://www.spinics.net/lists/kernel/msg2161934.html
>> applied to the tested tree?
> 
> Yes, linux-next 20160201 contains this patch.

Hmm, I will try to build this tree on UDOO board
today and see what happens.

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: remove explicit register defaults

[Maciej S. Szmigiero]

On 01.02.2016 13:25, Maciej S. Szmigiero wrote:
> On 01.02.2016 13:13, Fabio Estevam wrote:
>> Hi Maciej,
>>
>> On Mon, Feb 1, 2016 at 10:07 AM, Maciej S. Szmigiero
>> <m...@maciej.szmigiero.name> wrote:
>>> Is regmap patch from
>>> http://www.spinics.net/lists/kernel/msg2161934.html
>>> applied to the tested tree?
>>
>> Yes, linux-next 20160201 contains this patch.
> 
> Hmm, I will try to build this tree on UDOO board
> today and see what happens.

Looks like the problem occurs because commit
922a9f936e40 ("regmap: mmio: Convert to regmap_bus and fix accessor usage")
has removed ->read callback from MMIO regmap
implementation but it is used (via regmap_raw_read()) by
regcache code to initialize cache if reg default values
weren't provided explicitly.

If I revert this commit the SSI probes successfully again.

Looks like a possible solution would be to change
regmap_raw_read() to do read using _regmap_read in
case the cache is bypassed and there is no ->read
callback defined for regmap implementation.

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] ASoC: fsl_ssi: remove explicit register defaults

[Maciej S. Szmigiero]

There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

However, the cache needs to be fully populated at probe
time to avoid non-atomic allocations during register
access.

Special case here is imx21-class SSI, since
according to datasheet it don't have SACC{ST,EN,DIS}
regs.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support 
MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 42 ++
 1 file changed, 22 insertions(+), 20 deletions(-)

This replaces "ASoC: fsl_ssi: remove register defaults"
submission.

It may be good to delay merging this until commit
d51fe1f393a6 ("regmap: pass buffer size to regmap_raw_read() in 
regcache_hw_init()")
reaches ASoC tree since without that regmap fix
fsl_ssi will report error at probe time.

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..ed8de1035cda 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-   {CCSR_SSI_SCR, 0x},
-   {CCSR_SSI_SIER,0x3003},
-   {CCSR_SSI_STCR,0x0200},
-   {CCSR_SSI_SRCR,0x0200},
-   {CCSR_SSI_STCCR,   0x0004},
-   {CCSR_SSI_SRCCR,   0x0004},
-   {CCSR_SSI_SACNT,   0x},
-   {CCSR_SSI_STMSK,   0x},
-   {CCSR_SSI_SRMSK,   0x},
-   {CCSR_SSI_SACCEN,  0x},
-   {CCSR_SSI_SACCDIS, 0x},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+   .num_reg_defaults_raw = CCSR_SSI_SACCDIS / sizeof(uint32_t) + 1,
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
bool imx;
+   bool imx21regs; /* imx21-class SSI - no SACC{ST,EN,DIS} regs */
bool offline_config;
u32 sisr_write_mask;
 };
@@ -303,6 +289,7 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
.imx = true,
+   .imx21regs = true,
.offline_config = true,
.sisr_write_mask = 0,
 };
@@ -586,8 +573,12 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
 */
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+   if (!ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
 
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1388,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+   struct regmap_config regconfig = fsl_ssi_regconfig;
 
of_id = of_match_device(fsl_ssi_ids, >dev);
if (!of_id || !of_id->data)
@@ -1444,15 +1436,25 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(iomem);
ssi_private->ssi_phys = res->start;
 
+   if (ssi_private->soc->imx21regs) {
+   /*
+* According to datasheet imx21-class SSI
+* don't have SACC{ST,EN,DIS} regs.
+*/
+   regconfig.max_register = CCSR_SSI_SRMSK;
+   regconfig.num_reg_defaults_raw =
+   CCSR_SSI_SRMSK / sizeof(uint32_t) + 1;
+   }
+
ret = of_property_match_string(np, "clock-names", "ipg");
if (ret < 0) {
ssi_private->has_ipg_clk_name = false;
ssi_private->regs = devm_regmap_init_mmio(>dev, iomem,
-   _ssi_regconfig);
+   );
} else {
ssi_private->has_ipg_clk_name = true;
ssi_private->regs = devm_regmap_init_mmio_clk(>dev,
-   "ipg", 

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

Hi Fabio,

On 18.01.2016 13:51, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Sun, Jan 17, 2016 at 8:02 PM, Maciej S. Szmigiero
> <m...@maciej.szmigiero.name> wrote:
> 
>> Patch updated according to Timur's suggestions (needs regmap fix):
>> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> 
> Tested your patch against linux-next and it did not give me any warnings:
> 
> Tested-by: Fabio Estevam <fabio.este...@nxp.com>

Thanks for testing, I have submitted this version now.

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

On 17.01.2016 19:38, Timur Tabi wrote:
> Maciej S. Szmigiero wrote:
>> On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
>>> On 17.01.2016 06:16, Timur Tabi wrote:
>>>> Maciej S. Szmigiero wrote:
>>>>> This is because (at least according to the datasheet) imx21-class SSI
>>>>> registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
>>>>> reading them for cache initialization may not be safe.
>>>>>
>>>>> Also, a "MXC 91221 only" comment before these regs in FSL tree
>>>>> (drivers/mxc/ssi/registers.h) seems to confirm that these registers
>>>>> aren't present at least on some SSI (or SoC) models.
>>>>
>>>> Can't we just mark them as precious or something, so that we don't have to 
>>>> have two structures?
>>>
>>> Looks like it can be done with just one static regmap config struct
>>> used then as template - I will post updated patch.

Patch updated according to Timur's suggestions (needs regmap fix):
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..ed8de1035cda 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-   {CCSR_SSI_SCR, 0x},
-   {CCSR_SSI_SIER,0x3003},
-   {CCSR_SSI_STCR,0x0200},
-   {CCSR_SSI_SRCR,0x0200},
-   {CCSR_SSI_STCCR,   0x0004},
-   {CCSR_SSI_SRCCR,   0x0004},
-   {CCSR_SSI_SACNT,   0x},
-   {CCSR_SSI_STMSK,   0x},
-   {CCSR_SSI_SRMSK,   0x},
-   {CCSR_SSI_SACCEN,  0x},
-   {CCSR_SSI_SACCDIS, 0x},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+   .num_reg_defaults_raw = CCSR_SSI_SACCDIS / sizeof(uint32_t) + 1,
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
bool imx;
+   bool imx21regs; /* imx21-class SSI - no SACC{ST,EN,DIS} regs */
bool offline_config;
u32 sisr_write_mask;
 };
@@ -303,6 +289,7 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
.imx = true,
+   .imx21regs = true,
.offline_config = true,
.sisr_write_mask = 0,
 };
@@ -586,8 +573,12 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
 */
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+   /* no SACC{ST,EN,DIS} regs on imx21-class SSI */
+   if (!ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
 
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1388,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+   struct regmap_config regconfig = fsl_ssi_regconfig;
 
of_id = of_match_device(fsl_ssi_ids, >dev);
if (!of_id || !of_id->data)
@@ -1444,15 +1436,25 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(iomem);
ssi_private->ssi_phys = res->start;
 
+   if (ssi_private->soc->imx21regs) {
+   /*
+* According to datasheet imx21-class SSI
+* don't have SACC{ST,EN,DIS} regs.
+*/
+   regconfig.max_register = CCSR_SSI_SRMSK;
+   regconfig.num_reg_defaults_raw =
+   CCSR_SSI_SRMSK / sizeof(uint32_t) + 1;
+   }
+
ret = of_property_match_string(np, "clock-names", "ipg");
if (ret < 0) {
ssi_private->has_ipg_clk_name = false;
ssi_private->regs = devm_regmap_init_mmio(>dev, iomem,
-   _ssi_regconfig);
+   );
} else {
ssi_private->has_ipg_clk_name = true;
ssi_private->regs = devm_regmap_init_mmio_clk(>dev,
-   "ipg", iomem, _s

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

On 17.01.2016 06:16, Timur Tabi wrote:
> Maciej S. Szmigiero wrote:
>> This is because (at least according to the datasheet) imx21-class SSI
>> registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
>> reading them for cache initialization may not be safe.
>>
>> Also, a "MXC 91221 only" comment before these regs in FSL tree
>> (drivers/mxc/ssi/registers.h) seems to confirm that these registers
>> aren't present at least on some SSI (or SoC) models.
> 
> Can't we just mark them as precious or something, so that we don't have to 
> have two structures?

Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

On 17.01.2016 15:16, Maciej S. Szmigiero wrote:
> On 17.01.2016 06:16, Timur Tabi wrote:
>> Maciej S. Szmigiero wrote:
>>> This is because (at least according to the datasheet) imx21-class SSI
>>> registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
>>> reading them for cache initialization may not be safe.
>>>
>>> Also, a "MXC 91221 only" comment before these regs in FSL tree
>>> (drivers/mxc/ssi/registers.h) seems to confirm that these registers
>>> aren't present at least on some SSI (or SoC) models.
>>
>> Can't we just mark them as precious or something, so that we don't have to 
>> have two structures?
> 
> Looks like it can be done with just one static regmap config struct
> used then as template - I will post updated patch.

Updated patch:
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..105de76dd2fc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-   {CCSR_SSI_SCR, 0x},
-   {CCSR_SSI_SIER,0x3003},
-   {CCSR_SSI_STCR,0x0200},
-   {CCSR_SSI_SRCR,0x0200},
-   {CCSR_SSI_STCCR,   0x0004},
-   {CCSR_SSI_SRCCR,   0x0004},
-   {CCSR_SSI_SACNT,   0x},
-   {CCSR_SSI_STMSK,   0x},
-   {CCSR_SSI_SRMSK,   0x},
-   {CCSR_SSI_SACCEN,  0x},
-   {CCSR_SSI_SACCDIS, 0x},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+   .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
bool imx;
+   bool imx21regs;
bool offline_config;
u32 sisr_write_mask;
 };
@@ -295,6 +281,7 @@ struct fsl_ssi_private {
 
 static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
.imx = false,
+   .imx21regs = false,
.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -303,12 +290,14 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
.imx = true,
+   .imx21regs = true,
.offline_config = true,
.sisr_write_mask = 0,
 };
 
 static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
.imx = true,
+   .imx21regs = false,
.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -317,6 +306,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx51 = {
.imx = true,
+   .imx21regs = false,
.offline_config = false,
.sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1,
@@ -586,8 +576,11 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
 */
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+   if (!ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
 
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1390,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+   struct regmap_config regconfig = fsl_ssi_regconfig;
 
of_id = of_match_device(fsl_ssi_ids, >dev);
if (!of_id || !of_id->data)
@@ -1444,15 +1438,22 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(iomem);
ssi_private->ssi_phys = res->start;
 
+   if (ssi_private->soc->imx21regs) {
+   /* According to datasheet imx21-class SSI have less regs */
+   regconfig.max_register = CCSR_SSI_SRMSK;
+   regconfig.num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1;
+   }
+
ret = of_property_match_string(np, "clock-names", "ipg&qu

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

Hi Fabio,

On 11.01.2016 15:05, Fabio Estevam wrote:
> Hi Maciej,
> 
> On Mon, Jan 11, 2016 at 11:57 AM, Maciej S. Szmigiero
> <m...@maciej.szmigiero.name> wrote:
>> Hi Fabio,
> 
>> This will disable register cache so it isn't right.
>> Could you try REGCACHE_FLAT instead, please?
> 
> Yes, with REGCACHE_FLAT I don't get the warning.
> 
> Regards,

Is it possible for you to test the following updated patch?

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..16ee5745c992 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-   {CCSR_SSI_SCR, 0x},
-   {CCSR_SSI_SIER,0x3003},
-   {CCSR_SSI_STCR,0x0200},
-   {CCSR_SSI_SRCR,0x0200},
-   {CCSR_SSI_STCCR,   0x0004},
-   {CCSR_SSI_SRCCR,   0x0004},
-   {CCSR_SSI_SACNT,   0x},
-   {CCSR_SSI_STMSK,   0x},
-   {CCSR_SSI_SRMSK,   0x},
-   {CCSR_SSI_SACCEN,  0x},
-   {CCSR_SSI_SACCDIS, 0x},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -184,14 +170,27 @@ static bool fsl_ssi_writeable_reg(struct device *dev, 
unsigned int reg)
}
 }
 
+static const struct regmap_config fsl_ssi_regconfig_imx21 = {
+   .max_register = CCSR_SSI_SRMSK,
+   .reg_bits = 32,
+   .val_bits = 32,
+   .reg_stride = 4,
+   .val_format_endian = REGMAP_ENDIAN_NATIVE,
+   .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
+   .readable_reg = fsl_ssi_readable_reg,
+   .volatile_reg = fsl_ssi_volatile_reg,
+   .precious_reg = fsl_ssi_precious_reg,
+   .writeable_reg = fsl_ssi_writeable_reg,
+   .cache_type = REGCACHE_RBTREE,
+};
+
 static const struct regmap_config fsl_ssi_regconfig = {
.max_register = CCSR_SSI_SACCDIS,
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+   .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +200,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
 
 struct fsl_ssi_soc_data {
bool imx;
+   bool imx21regs;
bool offline_config;
u32 sisr_write_mask;
 };
@@ -295,6 +295,7 @@ struct fsl_ssi_private {
 
 static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
.imx = false,
+   .imx21regs = false,
.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -303,12 +304,14 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
.imx = true,
+   .imx21regs = true,
.offline_config = true,
.sisr_write_mask = 0,
 };
 
 static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
.imx = true,
+   .imx21regs = false,
.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -317,6 +320,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
 
 static struct fsl_ssi_soc_data fsl_ssi_imx51 = {
.imx = true,
+   .imx21regs = false,
.offline_config = false,
.sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1,
@@ -586,8 +590,11 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
 */
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+   if (!ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }
 
/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1404,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+   const struct regmap_config *regconfig;
 
of_id = of_match_device(fsl_ssi_ids, >dev);
if (!of_id || !of_id->data)
@@ -1444,15 +1452,21 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(iomem);
ssi_private->ssi_phys = res->start;
 
+   if (ssi_private->soc->imx21regs)
+   regconfig = _ssi_regconfig_im

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

On 17.01.2016 01:10, Timur Tabi wrote:
> Maciej S. Szmigiero wrote:
>> +static const struct regmap_config fsl_ssi_regconfig_imx21 = {
>> +.max_register = CCSR_SSI_SRMSK,
>> +.reg_bits = 32,
>> +.val_bits = 32,
>> +.reg_stride = 4,
>> +.val_format_endian = REGMAP_ENDIAN_NATIVE,
>> +.num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
>> +.readable_reg = fsl_ssi_readable_reg,
>> +.volatile_reg = fsl_ssi_volatile_reg,
>> +.precious_reg = fsl_ssi_precious_reg,
>> +.writeable_reg = fsl_ssi_writeable_reg,
>> +.cache_type = REGCACHE_RBTREE,
>> +};
>> +
>>   static const struct regmap_config fsl_ssi_regconfig = {
>>   .max_register = CCSR_SSI_SACCDIS,
>>   .reg_bits = 32,
>>   .val_bits = 32,
>>   .reg_stride = 4,
>>   .val_format_endian = REGMAP_ENDIAN_NATIVE,
>> -.reg_defaults = fsl_ssi_reg_defaults,
>> -.num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
>> +.num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
>>   .readable_reg = fsl_ssi_readable_reg,
>>   .volatile_reg = fsl_ssi_volatile_reg,
>>   .precious_reg = fsl_ssi_precious_reg,
> 
> Is this really necessary?  Why do we need separate register configs for one 
> specific SOC?
> There are already too many "if (some_stupid_imx_variant)" blocks in this 
> driver.

This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.

Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

Hi Fabio,

Thanks for testing.

On 11.01.2016 13:10, Fabio Estevam wrote:
> On Mon, Jan 11, 2016 at 10:04 AM, Fabio Estevam  wrote:
> 
>> This patch causes the following issue in linux-next:
>>
>> [2.526984] [ cut here ]
>> [2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755
>> lockdep_trace_alloc+0xf4/0x124()
>> [2.540771] DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags))
>> [2.546175] Modules linked in:
>> [2.549447] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
>> 4.4.0-rc8-next-20160111 #204
>> [2.557021] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>> [2.563553] Backtrace:
>> [2.566040] [] (dump_backtrace) from []
>> (show_stack+0x18/0x1c)
>> [2.573615]  r6:0ac3 r5: r4: r3:
>> [2.579362] [] (show_stack) from []
>> (dump_stack+0x88/0xa4)
>> [2.586607] [] (dump_stack) from []
>> (warn_slowpath_common+0x80/0xbc)
>> [2.594702]  r5:c0071ed0 r4:ef055b90
>> [2.598326] [] (warn_slowpath_common) from []
>> (warn_slowpath_fmt+0x38/0x40)
>> [2.607028]  r8:0004 r7:0004 r6:024080c0 r5:024080c0 r4:6093
>> [2.613829] [] (warn_slowpath_fmt) from []
>> (lockdep_trace_alloc+0xf4/0x124)
>> [2.622532]  r3:c09a1634 r2:c099dc0c
>> [2.626161] [] (lockdep_trace_alloc) from []
>> (kmem_cache_alloc+0x30/0x174)
>> [2.634778]  r4:ef001f00 r3:c0b02a88
>> [2.638407] [] (kmem_cache_alloc) from []
>> (regcache_rbtree_write+0x150/0x724)
>> [2.647283]  r10: r9:0010 r8:0004 r7:0004
>> r6:002c r5:
>> [2.655203]  r4:
>> [2.657767] [] (regcache_rbtree_write) from []
>> (regcache_write+0x5c/0x64)
> 
> This fixes the warning:
> 
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -180,7 +180,6 @@ static const struct regmap_config fsl_ssi_regconfig = {
> .volatile_reg = fsl_ssi_volatile_reg,
> .precious_reg = fsl_ssi_precious_reg,
> .writeable_reg = fsl_ssi_writeable_reg,
> -   .cache_type = REGCACHE_RBTREE,
>  };
> 
> Is this the correct fix?
> 

This will disable register cache so it isn't right.
Could you try REGCACHE_FLAT instead, please?

Looks like the problem here is rbtree cache does some non-atomic allocations in
read / write path when not supplied with default register values.

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

On 11.01.2016 15:00, Mark Brown wrote:
> On Mon, Jan 11, 2016 at 10:10:56AM -0200, Fabio Estevam wrote:
>> On Mon, Jan 11, 2016 at 10:04 AM, Fabio Estevam  wrote:
> 
>>> [2.526984] [ cut here ]
>>> [2.531632] WARNING: CPU: 1 PID: 1 at kernel/locking/lockdep.c:2755
>>> lockdep_trace_alloc+0xf4/0x124()
> 
>> This fixes the warning:
> 
>> --- a/sound/soc/fsl/fsl_ssi.c
>> +++ b/sound/soc/fsl/fsl_ssi.c
>> @@ -180,7 +180,6 @@ static const struct regmap_config fsl_ssi_regconfig = {
>> .volatile_reg = fsl_ssi_volatile_reg,
>> .precious_reg = fsl_ssi_precious_reg,
>> .writeable_reg = fsl_ssi_writeable_reg,
>> -   .cache_type = REGCACHE_RBTREE,
>>  };
> 
>> Is this the correct fix?
> 
> I suspect not, it looks like the driver is using the cache for
> suspend/resume handling.  I've dropped the patch for now.  Either the
> driver should explicitly write to the relevant registers outside of
> interrupt context to ensure the cache entry exists or it should keep the
> defaults and explicitly write them to hardware at startup to ensure
> sync (the former is more likely to be safe).

Is it acceptable to switch it to flat cache instead to not keep the register
defaults in driver?

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile

[Maciej S. Szmigiero]

Hi Timur,

Thanks for review.

On 10.01.2016 22:33, Timur Tabi wrote:
> Maciej S. Szmigiero wrote:
>> +regmap_write(regs, CCSR_SSI_SACNT,
>> +ssi_private->regcache_sacnt);
> 
> So I'm not familiar with all of the regcache features, but I understand this 
> patch.
> I was wondering if it makes sense to write the same exact value that was read 
> previously.
> Isn't it possible for the WR or RD bits to change between fsl_ssi_suspend() 
> and fsl_ssi_resume()?

These bits are only set in fsl_ssi_ac97_{read,write} which then wait 100usecs
before returning. This should be enough for SSI core to finish the relevant
operation and clear the bits again, so theoretically they shouldn't be set
outside these functions.

However, if AC'97 register access is done concurrently with suspend or resume
the read / written reg data might be corrupted.

It looks to me this is indeed possible since SSI PM callbacks are set in its
platform driver struct but ASoC core only calls PM callbacks in 
snd_soc_dai_driver
(which SSI driver don't set).

If I am correct with this reasoning then these callbacks need to be added to
snd_soc_dai_driver but platform driver ones should still be provided in case
the driver is loaded but the sound card is not yet registered.

I've CCed Zidan since he originally added PM support to this driver.

> That is, should we be doing this instead?
> 
> u32 temp;
> regmap_read(regs, CCSR_SSI_SACNT, );
> temp &= 0x18; // preserve WR and RD
> regmap_write(regs, CCSR_SSI_SACNT, (ssi_private->regcache_sacnt & ~0x18) | 
> temp);
> 

Maciej

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 2/3] ASoC: fsl_ssi: mark some registers precious

[Maciej S. Szmigiero]

Mark some registers precious since their
reads have side effects (like clearing flags).

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index cc22354d7758..40dfd8a36484 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -157,6 +157,21 @@ static bool fsl_ssi_volatile_reg(struct device *dev, 
unsigned int reg)
}
 }
 
+static bool fsl_ssi_precious_reg(struct device *dev, unsigned int reg)
+{
+   switch (reg) {
+   case CCSR_SSI_SRX0:
+   case CCSR_SSI_SRX1:
+   case CCSR_SSI_SISR:
+   case CCSR_SSI_SACADD:
+   case CCSR_SSI_SACDAT:
+   case CCSR_SSI_SATAG:
+   return true;
+   default:
+   return false;
+   }
+}
+
 static bool fsl_ssi_writeable_reg(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -179,6 +194,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
.num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
+   .precious_reg = fsl_ssi_precious_reg,
.writeable_reg = fsl_ssi_writeable_reg,
.cache_type = REGCACHE_RBTREE,
 };

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] ASoC: fsl-asoc-card: use different route map for AC'97 mode

[Maciej S. Szmigiero]

fsl_ssi uses different stream names ("AC97 Playback" / "AC97 Capture")
in AC'97 mode so in this case fsl-asoc-card route map should
also be using them.

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl-asoc-card.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index c63d89da51f1..562b3bd22d9a 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -107,6 +107,13 @@ static const struct snd_soc_dapm_route audio_map[] = {
{"CPU-Capture",  NULL, "Capture"},
 };
 
+static const struct snd_soc_dapm_route audio_map_ac97[] = {
+   {"AC97 Playback",  NULL, "ASRC-Playback"},
+   {"Playback",  NULL, "AC97 Playback"},
+   {"ASRC-Capture",  NULL, "AC97 Capture"},
+   {"AC97 Capture",  NULL, "Capture"},
+};
+
 /* Add all possible widgets into here without being redundant */
 static const struct snd_soc_dapm_widget fsl_asoc_card_dapm_widgets[] = {
SND_SOC_DAPM_LINE("Line Out Jack", NULL),
@@ -579,7 +586,8 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
priv->card.dev = >dev;
priv->card.name = priv->name;
priv->card.dai_link = priv->dai_link;
-   priv->card.dapm_routes = audio_map;
+   priv->card.dapm_routes = fsl_asoc_card_is_ac97(priv) ?
+audio_map_ac97 : audio_map;
priv->card.late_probe = fsl_asoc_card_late_probe;
priv->card.num_dapm_routes = ARRAY_SIZE(audio_map);
priv->card.dapm_widgets = fsl_asoc_card_dapm_widgets;

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile

[Maciej S. Szmigiero]

SACNT register should be marked volatile since
its WR and RD bits are cleared by SSI after
completing the relevant operation.
This unbreaks AC'97 register access.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support 
MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index e3abad5f980a..cc22354d7758 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -146,6 +146,7 @@ static bool fsl_ssi_volatile_reg(struct device *dev, 
unsigned int reg)
case CCSR_SSI_SRX1:
case CCSR_SSI_SISR:
case CCSR_SSI_SFCSR:
+   case CCSR_SSI_SACNT:
case CCSR_SSI_SACADD:
case CCSR_SSI_SACDAT:
case CCSR_SSI_SATAG:
@@ -239,8 +240,9 @@ struct fsl_ssi_private {
unsigned int baudclk_streams;
unsigned int bitclk_freq;
 
-   /*regcache for SFCSR*/
+   /* regcache for volatile regs */
u32 regcache_sfcsr;
+   u32 regcache_sacnt;
 
/* DMA params */
struct snd_dmaengine_dai_dma_data dma_params_tx;
@@ -1587,6 +1589,8 @@ static int fsl_ssi_suspend(struct device *dev)
 
regmap_read(regs, CCSR_SSI_SFCSR,
_private->regcache_sfcsr);
+   regmap_read(regs, CCSR_SSI_SACNT,
+   _private->regcache_sacnt);
 
regcache_cache_only(regs, true);
regcache_mark_dirty(regs);
@@ -1605,6 +1609,8 @@ static int fsl_ssi_resume(struct device *dev)
CCSR_SSI_SFCSR_RFWM1_MASK | CCSR_SSI_SFCSR_TFWM1_MASK |
CCSR_SSI_SFCSR_RFWM0_MASK | CCSR_SSI_SFCSR_TFWM0_MASK,
ssi_private->regcache_sfcsr);
+   regmap_write(regs, CCSR_SSI_SACNT,
+   ssi_private->regcache_sacnt);
 
return regcache_sync(regs);
 }
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Maciej S. Szmigiero]

There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support 
MEGA Fast")

Signed-off-by: Maciej S. Szmigiero <m...@maciej.szmigiero.name>
---
 sound/soc/fsl/fsl_ssi.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..f6ef7d64acb3 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
 };
 
-static const struct reg_default fsl_ssi_reg_defaults[] = {
-   {CCSR_SSI_SCR, 0x},
-   {CCSR_SSI_SIER,0x3003},
-   {CCSR_SSI_STCR,0x0200},
-   {CCSR_SSI_SRCR,0x0200},
-   {CCSR_SSI_STCCR,   0x0004},
-   {CCSR_SSI_SRCCR,   0x0004},
-   {CCSR_SSI_SACNT,   0x},
-   {CCSR_SSI_STMSK,   0x},
-   {CCSR_SSI_SRMSK,   0x},
-   {CCSR_SSI_SACCEN,  0x},
-   {CCSR_SSI_SACCDIS, 0x},
-};
-
 static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -190,8 +176,6 @@ static const struct regmap_config fsl_ssi_regconfig = {
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH v2] ASoC: fsl-asoc-card: add AC'97 support

[Maciej S. Szmigiero]

Add AC'97 support to fsl-asoc-card using generic
ASoC AC'97 CODEC.

The SSI controller will silently enable any TX
AC'97 slots that have their bits set in SLOTREQ
received from CODEC and then will redirect some
of playback samples there.

That's why it is important to make sure that
any of CODEC playback slots that can pull samples
are set to slots 3/4 (standard PCM playback slots).
Currently, this applies to S/PDIF slots as they
were seen to pull samples sometimes even with
S/PDIF output being disabled.

Signed-off-by: Maciej Szmigiero 
---
Changes from v1: don't use of_find_device_by_node() to
get pointer to I2S/PCM CODEC device as this only matches
platform bus devices while I2S/PCM CODECs supported by
this driver sit on I2C bus.

 .../devicetree/bindings/sound/fsl-asoc-card.txt|  10 +-
 sound/soc/fsl/fsl-asoc-card.c  | 140 -
 2 files changed, 116 insertions(+), 34 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt 
b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
index a96774c..ce55c0a 100644
--- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
@@ -13,13 +13,15 @@ So having this generic sound card allows all Freescale SoC 
users to benefit
 from the simplification of a new card support and the capability of the wide
 sample rates support through ASRC.
 
-Note: The card is initially designed for those sound cards who use I2S and
-  PCM DAI formats. However, it'll be also possible to support those non
-  I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as long
-  as the driver has been properly upgraded.
+Note: The card is initially designed for those sound cards who use AC'97, I2S
+  and PCM DAI formats. However, it'll be also possible to support those non
+  AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as
+  long as the driver has been properly upgraded.
 
 
 The compatible list for this generic sound card currently:
+ "fsl,imx-audio-ac97"
+
  "fsl,imx-audio-cs42888"
 
  "fsl,imx-audio-wm8962"
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index 0901d5e..1b05d1c 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -14,6 +14,9 @@
 #include 
 #include 
 #include 
+#if IS_ENABLED(CONFIG_SND_AC97_CODEC)
+#include 
+#endif
 #include 
 #include 
 
@@ -115,6 +118,11 @@ static const struct snd_soc_dapm_widget 
fsl_asoc_card_dapm_widgets[] = {
SND_SOC_DAPM_MIC("DMIC", NULL),
 };
 
+static bool fsl_asoc_card_is_ac97(struct fsl_asoc_card_priv *priv)
+{
+   return priv->dai_fmt == SND_SOC_DAIFMT_AC97;
+}
+
 static int fsl_asoc_card_hw_params(struct snd_pcm_substream *substream,
   struct snd_pcm_hw_params *params)
 {
@@ -133,7 +141,9 @@ static int fsl_asoc_card_hw_params(struct snd_pcm_substream 
*substream,
 * set_bias_level(), bypass the remaining settings in hw_params().
 * Note: (dai_fmt & CBM_CFM) includes CBM_CFM and CBM_CFS.
 */
-   if (priv->card.set_bias_level && priv->dai_fmt & SND_SOC_DAIFMT_CBM_CFM)
+   if ((priv->card.set_bias_level &&
+priv->dai_fmt & SND_SOC_DAIFMT_CBM_CFM) ||
+   fsl_asoc_card_is_ac97(priv))
return 0;
 
/* Specific configurations of DAIs starts from here */
@@ -300,7 +310,7 @@ static int fsl_asoc_card_audmux_init(struct device_node *np,
ext_port--;
 
/*
-* Use asynchronous mode (6 wires) for all cases.
+* Use asynchronous mode (6 wires) for all cases except AC97.
 * If only 4 wires are needed, just set SSI into
 * synchronous mode and enable 4 PADs in IOMUX.
 */
@@ -346,15 +356,30 @@ static int fsl_asoc_card_audmux_init(struct device_node 
*np,
   IMX_AUDMUX_V2_PTCR_TCLKDIR;
break;
default:
-   return -EINVAL;
+   if (!fsl_asoc_card_is_ac97(priv))
+   return -EINVAL;
+   }
+
+   if (fsl_asoc_card_is_ac97(priv)) {
+   int_ptcr = IMX_AUDMUX_V2_PTCR_SYN |
+  IMX_AUDMUX_V2_PTCR_TCSEL(ext_port) |
+  IMX_AUDMUX_V2_PTCR_TCLKDIR;
+   ext_ptcr = IMX_AUDMUX_V2_PTCR_SYN |
+  IMX_AUDMUX_V2_PTCR_TFSEL(int_port) |
+  IMX_AUDMUX_V2_PTCR_TFSDIR;
}
 
/* Asynchronous mode can not be set along with RCLKDIR */
-   ret = imx_audmux_v2_configure_port(int_port, 0,
-  IMX_AUDMUX_V2_PDCR_RXDSEL(ext_port));
-   if (ret) {
-   dev_err(dev, "audmux internal port setup failed\n");
-   return ret;
+   if (!fsl_asoc_card_is_ac97(priv)) {
+   unsigned int pdcr =
+   

[PATCH 1/2] ASoC: fsl-asoc-card: put ASRC OF node in case of unknown device

[Maciej S. Szmigiero]

In case of unknown DT compatible device the ASRC OF node
possibly acquired earlier by of_parse_phandle() has
to be put before returning from probe method.

Signed-off-by: Maciej Szmigiero 
---
 sound/soc/fsl/fsl-asoc-card.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index 5aeb6ed..96f55ae 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -488,7 +488,8 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
} else {
dev_err(>dev, "unknown Device Tree compatible\n");
-   return -EINVAL;
+   ret = -EINVAL;
+   goto asrc_fail;
}
 
/* Common settings for corresponding Freescale CPU DAI driver */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 2/2] ASoC: fsl-asoc-card: add AC'97 support

[Maciej S. Szmigiero]

Add AC'97 support to fsl-asoc-card using generic
ASoC AC'97 CODEC.

The SSI controller will silently enable any TX
AC'97 slots that have their bits set in SLOTREQ
received from CODEC and then will redirect some
of playback samples there.

That's why it is important to make sure that
any of CODEC playback slots that can pull samples
are set to slots 3/4 (standard PCM playback slots).
Currently, this applies to S/PDIF slots as they
were seen to pull samples sometimes even with
S/PDIF output being disabled.

Signed-off-by: Maciej Szmigiero 
---
 .../devicetree/bindings/sound/fsl-asoc-card.txt|   10 +-
 sound/soc/fsl/fsl-asoc-card.c  |  145 +++
 2 files changed, 120 insertions(+), 35 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt 
b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
index a96774c..ce55c0a 100644
--- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
@@ -13,13 +13,15 @@ So having this generic sound card allows all Freescale SoC 
users to benefit
 from the simplification of a new card support and the capability of the wide
 sample rates support through ASRC.
 
-Note: The card is initially designed for those sound cards who use I2S and
-  PCM DAI formats. However, it'll be also possible to support those non
-  I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as long
-  as the driver has been properly upgraded.
+Note: The card is initially designed for those sound cards who use AC'97, I2S
+  and PCM DAI formats. However, it'll be also possible to support those non
+  AC'97/I2S/PCM type sound cards, such as S/PDIF audio and HDMI audio, as
+  long as the driver has been properly upgraded.
 
 
 The compatible list for this generic sound card currently:
+ "fsl,imx-audio-ac97"
+
  "fsl,imx-audio-cs42888"
 
  "fsl,imx-audio-wm8962"
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index 96f55ae..385a9f8 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -14,6 +14,9 @@
 #include 
 #include 
 #include 
+#if IS_ENABLED(CONFIG_SND_AC97_CODEC)
+#include 
+#endif
 #include 
 #include 
 
@@ -115,6 +118,11 @@ static const struct snd_soc_dapm_widget 
fsl_asoc_card_dapm_widgets[] = {
SND_SOC_DAPM_MIC("DMIC", NULL),
 };
 
+static bool fsl_asoc_card_is_ac97(struct fsl_asoc_card_priv *priv)
+{
+   return priv->dai_fmt == SND_SOC_DAIFMT_AC97;
+}
+
 static int fsl_asoc_card_hw_params(struct snd_pcm_substream *substream,
   struct snd_pcm_hw_params *params)
 {
@@ -133,7 +141,9 @@ static int fsl_asoc_card_hw_params(struct snd_pcm_substream 
*substream,
 * set_bias_level(), bypass the remaining settings in hw_params().
 * Note: (dai_fmt & CBM_CFM) includes CBM_CFM and CBM_CFS.
 */
-   if (priv->card.set_bias_level && priv->dai_fmt & SND_SOC_DAIFMT_CBM_CFM)
+   if ((priv->card.set_bias_level &&
+priv->dai_fmt & SND_SOC_DAIFMT_CBM_CFM) ||
+   fsl_asoc_card_is_ac97(priv))
return 0;
 
/* Specific configurations of DAIs starts from here */
@@ -300,7 +310,7 @@ static int fsl_asoc_card_audmux_init(struct device_node *np,
ext_port--;
 
/*
-* Use asynchronous mode (6 wires) for all cases.
+* Use asynchronous mode (6 wires) for all cases except AC97.
 * If only 4 wires are needed, just set SSI into
 * synchronous mode and enable 4 PADs in IOMUX.
 */
@@ -346,15 +356,30 @@ static int fsl_asoc_card_audmux_init(struct device_node 
*np,
   IMX_AUDMUX_V2_PTCR_TCLKDIR;
break;
default:
-   return -EINVAL;
+   if (!fsl_asoc_card_is_ac97(priv))
+   return -EINVAL;
+   }
+
+   if (fsl_asoc_card_is_ac97(priv)) {
+   int_ptcr = IMX_AUDMUX_V2_PTCR_SYN |
+  IMX_AUDMUX_V2_PTCR_TCSEL(ext_port) |
+  IMX_AUDMUX_V2_PTCR_TCLKDIR;
+   ext_ptcr = IMX_AUDMUX_V2_PTCR_SYN |
+  IMX_AUDMUX_V2_PTCR_TFSEL(int_port) |
+  IMX_AUDMUX_V2_PTCR_TFSDIR;
}
 
/* Asynchronous mode can not be set along with RCLKDIR */
-   ret = imx_audmux_v2_configure_port(int_port, 0,
-  IMX_AUDMUX_V2_PDCR_RXDSEL(ext_port));
-   if (ret) {
-   dev_err(dev, "audmux internal port setup failed\n");
-   return ret;
+   if (!fsl_asoc_card_is_ac97(priv)) {
+   unsigned int pdcr =
+   IMX_AUDMUX_V2_PDCR_RXDSEL(ext_port);
+
+   ret = imx_audmux_v2_configure_port(int_port, 0,
+  pdcr);
+   if (ret) {
+   

[PATCH 1/6][RESEND] ASoC: fsl_ssi: enable IPG clock during AC'97 reg access

[Maciej S. Szmigiero]

IPG clock have to be enabled during AC'97 CODEC register
access in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
This is a resend without changes, to keep the whole series
together.

 sound/soc/fsl/fsl_ssi.c |   19 +++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 484ff20..8185edc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1127,10 +1127,17 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
struct regmap *regs = fsl_ac97_data-regs;
unsigned int lreg;
unsigned int lval;
+   int ret;
 
if (reg  0x7f)
return;
 
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 write clk_prepare_enable failed: %d\n,
+   ret);
+   return;
+   }
 
lreg = reg   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1141,6 +1148,8 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
regmap_update_bits(regs, CCSR_SSI_SACNT, CCSR_SSI_SACNT_RDWR_MASK,
CCSR_SSI_SACNT_WR);
udelay(100);
+
+   clk_disable_unprepare(fsl_ac97_data-clk);
 }
 
 static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
@@ -1151,6 +1160,14 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
unsigned short val = -1;
u32 reg_val;
unsigned int lreg;
+   int ret;
+
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 read clk_prepare_enable failed: %d\n,
+   ret);
+   return -1;
+   }
 
lreg = (reg  0x7f)   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1162,6 +1179,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
regmap_read(regs, CCSR_SSI_SACDAT, reg_val);
val = (reg_val  4)  0x;
 
+   clk_disable_unprepare(fsl_ac97_data-clk);
+
return val;
 }
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 2/6][RESEND] ASoC: fsl_ssi: AC'97 DAI driver needs probe method too

[Maciej S. Szmigiero]

AC'97 DAI driver struct need the same probe method as
I2S one to setup DMA params in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
This is a resend without changes, to keep the whole series
together.

 sound/soc/fsl/fsl_ssi.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 8185edc..a83b900 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1101,6 +1101,7 @@ static const struct snd_soc_component_driver 
fsl_ssi_component = {
 
 static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.bus_control = true,
+   .probe = fsl_ssi_dai_probe,
.playback = {
.stream_name = AC97 Playback,
.channels_min = 2,

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 3/6][RESEND] ASoC: fsl_ssi: enable AC'97 asymmetric rates

[Maciej S. Szmigiero]

AC'97 bus can support asymmetric playback/capture rates
so enable them in this case in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
This is a resend without changes, to keep the whole series
together.

 sound/soc/fsl/fsl_ssi.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index a83b900..7f4f0b9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
-   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+
ssi_private-cpu_dai_drv.symmetric_channels = 1;
ssi_private-cpu_dai_drv.symmetric_samplebits = 1;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 5/6][RESEND] ASoC: fsl_ssi: instantiate AC'97 CODEC

[Maciej S. Szmigiero]

Instantiate AC'97 CODEC in fsl_ssi driver AC'97 mode.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
This is a resend without changes, to keep the whole series
together.

 sound/soc/fsl/fsl_ssi.c |   21 +
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 154bcf6..8e5ff5e 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1460,6 +1460,27 @@ done:
_fsl_ssi_set_dai_fmt(pdev-dev, ssi_private,
 ssi_private-dai_fmt);
 
+   if (fsl_ssi_is_ac97(ssi_private)) {
+   u32 ssi_idx;
+
+   ret = of_property_read_u32(np, cell-index, ssi_idx);
+   if (ret) {
+   dev_err(pdev-dev, cannot get SSI index property\n);
+   goto error_sound_card;
+   }
+
+   ssi_private-pdev =
+   platform_device_register_data(NULL,
+   ac97-codec, ssi_idx, NULL, 0);
+   if (IS_ERR(ssi_private-pdev)) {
+   ret = PTR_ERR(ssi_private-pdev);
+   dev_err(pdev-dev,
+   failed to register AC97 codec platform: %d\n,
+   ret);
+   goto error_sound_card;
+   }
+   }
+
return 0;
 
 error_sound_card:

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 6/6 v2][RESEND] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

[Maciej S. Szmigiero]

Adjust set DAI format function in fsl_ssi driver
so it doesn't fail and clears RXDIR in AC'97 mode.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
Changes from v1: fix indentation to be consistent with rest
of the driver.

 sound/soc/fsl/fsl_ssi.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 8e5ff5e..1ba63bd 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
break;
default:
-   return -EINVAL;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   return -EINVAL;
}
 
stcr |= strcr;
srcr |= strcr;
 
-   if (ssi_private-cpu_dai_drv.symmetric_rates) {
-   /* Need to clear RXDIR when using SYNC mode */
+   if (ssi_private-cpu_dai_drv.symmetric_rates
+   || fsl_ssi_is_ac97(ssi_private)) {
+   /* Need to clear RXDIR when using SYNC or AC97 mode */
srcr = ~CCSR_SSI_SRCR_RXDIR;
scr |= CCSR_SSI_SCR_SYN;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 4/6][RESEND] ASoC: fsl_ssi: add AC'97 ops setting check and cleanup

[Maciej S. Szmigiero]

Check whether setting AC'97 ops succeeded and clean them
on removal so the fsl_ssi driver can be reloaded.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
This is a resend without changes, to keep the whole series
together.

 sound/soc/fsl/fsl_ssi.c |9 -
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 7f4f0b9..154bcf6 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1340,7 +1340,11 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
fsl_ac97_data = ssi_private;
 
-   snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   ret = snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   if (ret) {
+   dev_err(pdev-dev, could not set AC'97 ops\n);
+   return ret;
+   }
} else {
/* Initialize this copy of the CPU DAI driver structure */
memcpy(ssi_private-cpu_dai_drv, fsl_ssi_dai_template,
@@ -1480,6 +1484,9 @@ static int fsl_ssi_remove(struct platform_device *pdev)
if (ssi_private-soc-imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
+   if (fsl_ssi_is_ac97(ssi_private))
+   snd_soc_set_ac97_ops(NULL);
+
return 0;
 }
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 6/6 v2] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

[Maciej S. Szmigiero]

On 03.08.2015 18:21, Mark Brown wrote:
 On Mon, Aug 03, 2015 at 12:44:11AM +0200, Maciej S. Szmigiero wrote:
 Adjust set DAI format function in fsl_ssi driver
 so it doesn't fail and clears RXDIR in AC'97 mode.

 Changes from v1: fix indentation to be consistent with rest
 of the driver.
 
 Inter version changelogs go after the --- as covered in
 SubmittingPatches.
 

 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 
 Please don't bury new patches in reply to old submissions, especially
 not individual patches - it makes it hard to work out what's going on
 and make sure that the most current version of everything is being
 applied.  In order to avoid confusion I'm not going to look at these,
 please resubmit as a new thread.
 
 Please also try to thread your patch series together (git send-email can
 do this for you) - it also helps people keep track of things.

Thanks for information.

In cases like this where only one patch of six patch series is updated
should other ones be resubmitted as well to keep the full patch series
together?

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 6/6 v2] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

[Maciej S. Szmigiero]

Adjust set DAI format function in fsl_ssi driver
so it doesn't fail and clears RXDIR in AC'97 mode.

Changes from v1: fix indentation to be consistent with rest
of the driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 8e5ff5e..1ba63bd 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
break;
default:
-   return -EINVAL;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   return -EINVAL;
}
 
stcr |= strcr;
srcr |= strcr;
 
-   if (ssi_private-cpu_dai_drv.symmetric_rates) {
-   /* Need to clear RXDIR when using SYNC mode */
+   if (ssi_private-cpu_dai_drv.symmetric_rates
+   || fsl_ssi_is_ac97(ssi_private)) {
+   /* Need to clear RXDIR when using SYNC or AC97 mode */
srcr = ~CCSR_SSI_SRCR_RXDIR;
scr |= CCSR_SSI_SCR_SYN;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 5/6] ASoC: fsl_ssi: instantiate AC'97 CODEC

[Maciej S. Szmigiero]

On 31.07.2015 07:46, Markus Pargmann wrote:
 On Thu, Jul 30, 2015 at 04:35:23PM +0200, Maciej S. Szmigiero wrote:
 Instantiate AC'97 CODEC in fsl_ssi driver AC'97 mode.

 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 ---
  sound/soc/fsl/fsl_ssi.c |   21 +
  1 files changed, 21 insertions(+), 0 deletions(-)

 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 154bcf6..8e5ff5e 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -1460,6 +1460,27 @@ done:
  _fsl_ssi_set_dai_fmt(pdev-dev, ssi_private,
   ssi_private-dai_fmt);
  
 +if (fsl_ssi_is_ac97(ssi_private)) {
 +u32 ssi_idx;
 +
 +ret = of_property_read_u32(np, cell-index, ssi_idx);
 
 This property is not set anywhere in the imx* DTs.

That's right, but it is documented as required property in sound/fsl,ssi.txt:
 Required properties:
 (..)
 - cell-index:   The SSI, 0 = SSI1, 1 = SSI2, and so on.

 
 +if (ret) {
 +dev_err(pdev-dev, cannot get SSI index property\n);
 +goto error_sound_card;
 +}
 +
 +ssi_private-pdev =
 +platform_device_register_data(NULL,
 +ac97-codec, ssi_idx, NULL, 0);
 
 If you really want to create a device at this point you should use
 PLATFORM_DEVID_AUTO. I would prefer something where this is created in
 DT. On the other side it is a discoverable bus..

In the original implementation the codec was instantiated in DT but
the feedback was that this is wrong since devices on discoverable
buses shouldn't be in DT.

The platform device index is based on SSI index since the sound
card driver (fsl-asoc-card) has to know CODEC device name to identify
it in DAI links - as the only identification options seem to be either
DT node of CODEC or its device name.

That's why the (platform) device name has to be deterministic
if there is no mechanism to pass it from controller driver
to sound card driver.

 Regards,
 
 Markus

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 6/6] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

[Maciej S. Szmigiero]

On 31.07.2015 07:58, Markus Pargmann wrote:
 On Thu, Jul 30, 2015 at 04:35:58PM +0200, Maciej S. Szmigiero wrote:
 Adjust set DAI format function in fsl_ssi driver so it
 doesn't fail and clears RXDIR in AC'97 mode.

 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 ---
  sound/soc/fsl/fsl_ssi.c |8 +---
  1 files changed, 5 insertions(+), 3 deletions(-)

 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 8e5ff5e..37aabe3 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
  scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
  break;
  default:
 -return -EINVAL;
 +if (!fsl_ssi_is_ac97(ssi_private))
 +return -EINVAL;
 
 I think it would be better to add another case for the other mode which
 is supported (AC97) instead of using the default case.

This is a switch of DAI clock masters and AC'97 is none of them:
while case 0: can be added this would be very similar to the current code.

Alternatively, the whole switch statement could be wrapped inside
if (!fsl_ssi_is_ac97(ssi_private)) if that would be better
with regards to code style.

  }
  
  stcr |= strcr;
  srcr |= strcr;
  
 -if (ssi_private-cpu_dai_drv.symmetric_rates) {
 -/* Need to clear RXDIR when using SYNC mode */
 +if (ssi_private-cpu_dai_drv.symmetric_rates
 +|| fsl_ssi_is_ac97(ssi_private)) {
 
 Please fix this indention. Most of the driver is written with 2 tab
 indention after a line break and the new policy seems to be to indent on
 the opening bracket.

Will reindent this.

 
 Regards,
 
 Markus

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

[Maciej S. Szmigiero]

Hi Markus,

Thanks for looking into the changes.

On 31.07.2015 07:53, Markus Pargmann wrote:
 On Fri, Jul 31, 2015 at 07:27:19AM +0200, Markus Pargmann wrote:
 Hi,

 On Thu, Jul 30, 2015 at 04:34:19PM +0200, Maciej S. Szmigiero wrote:
 AC'97 bus can support asymmetric playback/capture rates
 so enable them in this case in fsl_ssi driver.

 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 ---
  sound/soc/fsl/fsl_ssi.c |4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)

 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index a83b900..7f4f0b9 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
  
 /* Are the RX and the TX clocks locked? */
 if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
 -   ssi_private-cpu_dai_drv.symmetric_rates = 1;
 +   if (!fsl_ssi_is_ac97(ssi_private))
 +   ssi_private-cpu_dai_drv.symmetric_rates = 1;
 +

 Why don't you use the DT property that is parsed here to enable
 asymmetric rates?

Because in the AC'97 mode the driver supports only one channel config and one 
sample format
anyway the remaining settings controlled by this property don't do anything.

Since it should be safe to enable asymmetric rates with any AC'97 CODEC I think 
it is good
to do it in driver than to add fsl,ssi-asynchronous to every AC'97 DT node.

Also the description of this property in fsl,ssi.txt looks like that it only 
makes sense in
non-AC'97 mode.

 Just found the last version of this series. Please use v2 and describe
 changes for a new iteration of a series.

This is just a resubmission, there are no functional differences since
it was originally submitted a month ago. 

 There is also a different setup with AC97 which does not use DMA. See
 the long comment at the top of the file about how ac97 is already used.

I understand that the problem with FIQ handler described in comment
on top of fsl_ssi.c only pertains incoming data, that is capture.

This patch effectively does not touch capture rates as they are already
limited to 48kHz only since this is the only capture rate defined in
fsl_ssi AC'97 DAI driver capture capabilities.

 Regards,
 
 Markus

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

[Maciej S. Szmigiero]

AC'97 bus can support asymmetric playback/capture rates
so enable them in this case in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index a83b900..7f4f0b9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
-   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+
ssi_private-cpu_dai_drv.symmetric_channels = 1;
ssi_private-cpu_dai_drv.symmetric_samplebits = 1;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 6/6] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

[Maciej S. Szmigiero]

Adjust set DAI format function in fsl_ssi driver so it
doesn't fail and clears RXDIR in AC'97 mode.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 8e5ff5e..37aabe3 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
break;
default:
-   return -EINVAL;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   return -EINVAL;
}
 
stcr |= strcr;
srcr |= strcr;
 
-   if (ssi_private-cpu_dai_drv.symmetric_rates) {
-   /* Need to clear RXDIR when using SYNC mode */
+   if (ssi_private-cpu_dai_drv.symmetric_rates
+   || fsl_ssi_is_ac97(ssi_private)) {
+   /* Need to clear RXDIR when using SYNC or AC97 mode */
srcr = ~CCSR_SSI_SRCR_RXDIR;
scr |= CCSR_SSI_SCR_SYN;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 5/6] ASoC: fsl_ssi: instantiate AC'97 CODEC

[Maciej S. Szmigiero]

Instantiate AC'97 CODEC in fsl_ssi driver AC'97 mode.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |   21 +
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 154bcf6..8e5ff5e 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1460,6 +1460,27 @@ done:
_fsl_ssi_set_dai_fmt(pdev-dev, ssi_private,
 ssi_private-dai_fmt);
 
+   if (fsl_ssi_is_ac97(ssi_private)) {
+   u32 ssi_idx;
+
+   ret = of_property_read_u32(np, cell-index, ssi_idx);
+   if (ret) {
+   dev_err(pdev-dev, cannot get SSI index property\n);
+   goto error_sound_card;
+   }
+
+   ssi_private-pdev =
+   platform_device_register_data(NULL,
+   ac97-codec, ssi_idx, NULL, 0);
+   if (IS_ERR(ssi_private-pdev)) {
+   ret = PTR_ERR(ssi_private-pdev);
+   dev_err(pdev-dev,
+   failed to register AC97 codec platform: %d\n,
+   ret);
+   goto error_sound_card;
+   }
+   }
+
return 0;
 
 error_sound_card:

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 1/6] ASoC: fsl_ssi: enable IPG clock during AC'97 reg access

[Maciej S. Szmigiero]

IPG clock have to be enabled during AC'97 CODEC register
access in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |   19 +++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 484ff20..8185edc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1127,10 +1127,17 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
struct regmap *regs = fsl_ac97_data-regs;
unsigned int lreg;
unsigned int lval;
+   int ret;
 
if (reg  0x7f)
return;
 
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 write clk_prepare_enable failed: %d\n,
+   ret);
+   return;
+   }
 
lreg = reg   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1141,6 +1148,8 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
regmap_update_bits(regs, CCSR_SSI_SACNT, CCSR_SSI_SACNT_RDWR_MASK,
CCSR_SSI_SACNT_WR);
udelay(100);
+
+   clk_disable_unprepare(fsl_ac97_data-clk);
 }
 
 static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
@@ -1151,6 +1160,14 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
unsigned short val = -1;
u32 reg_val;
unsigned int lreg;
+   int ret;
+
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 read clk_prepare_enable failed: %d\n,
+   ret);
+   return -1;
+   }
 
lreg = (reg  0x7f)   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1162,6 +1179,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
regmap_read(regs, CCSR_SSI_SACDAT, reg_val);
val = (reg_val  4)  0x;
 
+   clk_disable_unprepare(fsl_ac97_data-clk);
+
return val;
 }
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 4/6] ASoC: fsl_ssi: add AC'97 ops setting check and cleanup

[Maciej S. Szmigiero]

Check whether setting AC'97 ops succeeded and clean them
on removal so the fsl_ssi driver can be reloaded.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |9 -
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 7f4f0b9..154bcf6 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1340,7 +1340,11 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
fsl_ac97_data = ssi_private;
 
-   snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   ret = snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   if (ret) {
+   dev_err(pdev-dev, could not set AC'97 ops\n);
+   return ret;
+   }
} else {
/* Initialize this copy of the CPU DAI driver structure */
memcpy(ssi_private-cpu_dai_drv, fsl_ssi_dai_template,
@@ -1480,6 +1484,9 @@ static int fsl_ssi_remove(struct platform_device *pdev)
if (ssi_private-soc-imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
+   if (fsl_ssi_is_ac97(ssi_private))
+   snd_soc_set_ac97_ops(NULL);
+
return 0;
 }
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 2/6] ASoC: fsl_ssi: AC'97 DAI driver needs probe method too

[Maciej S. Szmigiero]

AC'97 DAI driver struct need the same probe method as
I2S one to setup DMA params in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 8185edc..a83b900 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1101,6 +1101,7 @@ static const struct snd_soc_component_driver 
fsl_ssi_component = {
 
 static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.bus_control = true,
+   .probe = fsl_ssi_dai_probe,
.playback = {
.stream_name = AC97 Playback,
.channels_min = 2,

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/6] ASoC: fsl_ssi: enable IPG clock during AC'97 reg access

[Maciej S. Szmigiero]

Hi Fabio,

On 30.07.2015 17:20, Fabio Estevam wrote:
 Hi Maciej,
 
 On Thu, Jul 30, 2015 at 11:33 AM, Maciej S. Szmigiero
 m...@maciej.szmigiero.name wrote:
 
  static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
 @@ -1151,6 +1160,14 @@ static unsigned short fsl_ssi_ac97_read(struct 
 snd_ac97 *ac97,
 unsigned short val = -1;
 u32 reg_val;
 unsigned int lreg;
 +   int ret;
 +
 +   ret = clk_prepare_enable(fsl_ac97_data-clk);
 +   if (ret) {
 +   pr_err(ac97 read clk_prepare_enable failed: %d\n,
 +   ret);
 +   return -1;
 
 return ret, please.
 

This function normal return value is an AC'97 register value,
so isn't more appropriate to return 0x in case of error
than linux error code?

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: fix AC'97 mode

[Maciej S. Szmigiero]

W dniu 28.06.2015 06:27, Timur Tabi pisze:
 Maciej S. Szmigiero wrote:
 +if (newbinding  fsl_ssi_is_ac97(ssi_private)) {
 
 Is the newbinding necessary?  I thought only the original PowerPC device 
 trees were the only one that have the old binding, and they never supported 
 AC97.

If there isn't going to be new platforms added with old bindings then this 
won't be needed - I'll remove it.

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 1/6] ASoC: fsl_ssi: enable IPG clock during AC'97 reg access

[Maciej S. Szmigiero]

IPG clock have to be enabled during AC'97 CODEC register
access in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |   19 +++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index c7647e0..9c46c7d 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1127,10 +1127,17 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
struct regmap *regs = fsl_ac97_data-regs;
unsigned int lreg;
unsigned int lval;
+   int ret;
 
if (reg  0x7f)
return;
 
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 write clk_prepare_enable failed: %d\n,
+   ret);
+   return;
+   }
 
lreg = reg   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1141,6 +1148,8 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
regmap_update_bits(regs, CCSR_SSI_SACNT, CCSR_SSI_SACNT_RDWR_MASK,
CCSR_SSI_SACNT_WR);
udelay(100);
+
+   clk_disable_unprepare(fsl_ac97_data-clk);
 }
 
 static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
@@ -1151,6 +1160,14 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
unsigned short val = -1;
u32 reg_val;
unsigned int lreg;
+   int ret;
+
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 read clk_prepare_enable failed: %d\n,
+   ret);
+   return -1;
+   }
 
lreg = (reg  0x7f)   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1162,6 +1179,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
regmap_read(regs, CCSR_SSI_SACDAT, reg_val);
val = (reg_val  4)  0x;
 
+   clk_disable_unprepare(fsl_ac97_data-clk);
+
return val;
 }
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

[Maciej S. Szmigiero]

AC'97 bus can support asymmetric playback/capture rates
so enable them in this case in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 2ce9e1d..f3034b9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
-   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+
ssi_private-cpu_dai_drv.symmetric_channels = 1;
ssi_private-cpu_dai_drv.symmetric_samplebits = 1;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 2/6] ASoC: fsl_ssi: AC'97 DAI driver needs probe method too

[Maciej S. Szmigiero]

AC'97 DAI driver struct need the same probe method as
I2S one to setup DMA params in fsl_ssi driver.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 9c46c7d..2ce9e1d 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1101,6 +1101,7 @@ static const struct snd_soc_component_driver 
fsl_ssi_component = {
 
 static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.bus_control = true,
+   .probe = fsl_ssi_dai_probe,
.playback = {
.stream_name = AC97 Playback,
.channels_min = 2,

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 6/6] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

[Maciej S. Szmigiero]

Adjust set DAI format function in fsl_ssi driver so it
doesn't fail and clears RXDIR in AC'97 mode.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index e79dc16..d043c7c 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
break;
default:
-   return -EINVAL;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   return -EINVAL;
}
 
stcr |= strcr;
srcr |= strcr;
 
-   if (ssi_private-cpu_dai_drv.symmetric_rates) {
-   /* Need to clear RXDIR when using SYNC mode */
+   if (ssi_private-cpu_dai_drv.symmetric_rates
+   || fsl_ssi_is_ac97(ssi_private)) {
+   /* Need to clear RXDIR when using SYNC or AC97 mode */
srcr = ~CCSR_SSI_SRCR_RXDIR;
scr |= CCSR_SSI_SCR_SYN;
}

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 5/6] ASoC: fsl_ssi: instantiate AC'97 CODEC

[Maciej S. Szmigiero]

Instantiate AC'97 CODEC in fsl_ssi driver AC'97 mode.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |   21 +
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 0b4fcd9..e79dc16 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1460,6 +1460,27 @@ done:
_fsl_ssi_set_dai_fmt(pdev-dev, ssi_private,
 ssi_private-dai_fmt);
 
+   if (fsl_ssi_is_ac97(ssi_private)) {
+   u32 ssi_idx;
+
+   ret = of_property_read_u32(np, cell-index, ssi_idx);
+   if (ret) {
+   dev_err(pdev-dev, cannot get SSI index property\n);
+   goto error_sound_card;
+   }
+
+   ssi_private-pdev =
+   platform_device_register_data(NULL,
+   ac97-codec, ssi_idx, NULL, 0);
+   if (IS_ERR(ssi_private-pdev)) {
+   ret = PTR_ERR(ssi_private-pdev);
+   dev_err(pdev-dev,
+   failed to register AC97 codec platform: %d\n,
+   ret);
+   goto error_sound_card;
+   }
+   }
+
return 0;
 
 error_sound_card:

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH 4/6] ASoC: fsl_ssi: add AC'97 ops setting check and cleanup

[Maciej S. Szmigiero]

Check whether setting AC'97 ops succeeded and clean them
on removal so the fsl_ssi driver can be reloaded.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
---
 sound/soc/fsl/fsl_ssi.c |9 -
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index f3034b9..0b4fcd9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -1340,7 +1340,11 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
fsl_ac97_data = ssi_private;
 
-   snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   ret = snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   if (ret) {
+   dev_err(pdev-dev, could not set AC'97 ops\n);
+   return ret;
+   }
} else {
/* Initialize this copy of the CPU DAI driver structure */
memcpy(ssi_private-cpu_dai_drv, fsl_ssi_dai_template,
@@ -1480,6 +1484,9 @@ static int fsl_ssi_remove(struct platform_device *pdev)
if (ssi_private-soc-imx)
fsl_ssi_imx_clean(pdev, ssi_private);
 
+   if (fsl_ssi_is_ac97(ssi_private))
+   snd_soc_set_ac97_ops(NULL);
+
return 0;
 }
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

[Maciej S. Szmigiero]

W dniu 28.06.2015 16:01, Timur Tabi pisze:
 Maciej S. Szmigiero wrote:
   /* Are the RX and the TX clocks locked? */
   if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
 -ssi_private-cpu_dai_drv.symmetric_rates = 1;
 +if (!fsl_ssi_is_ac97(ssi_private))
 +ssi_private-cpu_dai_drv.symmetric_rates = 1;
 +
 
 Is this necessary?  Why not just add fsl,ssi-asynchronous to the AC97 device 
 tree node?

Because in the AC'97 mode the driver supports only one channel config and one 
sample format
anyway the remaining settings controlled by this property don't do anything.

Since it should be safe to enable asymmetric rates with any AC'97 CODEC I think 
it is good
to do it in driver than to add fsl,ssi-asynchronous to every AC'97 DT node.

Also the description of this property in fsl,ssi.txt looks like that it only 
makes sense in
non-AC'97 mode.

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] ASoC: fsl_ssi: fix AC'97 mode

[Maciej S. Szmigiero]

Currently the AC'97 mode in fsl_ssi driver isn't functional.

This patch implements the following changes to make it work
properly:
* IPG clock have to be enabled during AC'97 CODEC
register access,
* AC'97 DAI driver struct need the same probe method as
I2S one to setup DMA params,
* AC'97 bus can support asymmetric playback/capture rates,
* Check whether setting AC'97 ops succeeded and
clean them on removal so the driver can be reloaded,
* AC'97 CODEC will be instantiated in AC'97 mode,
* Set DAI format function small fixes in AC'97 mode.

Tested on UDOO board.

Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index c7647e0..9a63df2 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
break;
default:
-   return -EINVAL;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   return -EINVAL;
}
 
stcr |= strcr;
srcr |= strcr;
 
-   if (ssi_private-cpu_dai_drv.symmetric_rates) {
-   /* Need to clear RXDIR when using SYNC mode */
+   if (ssi_private-cpu_dai_drv.symmetric_rates
+   || fsl_ssi_is_ac97(ssi_private)) {
+   /* Need to clear RXDIR when using SYNC or AC97 mode */
srcr = ~CCSR_SSI_SRCR_RXDIR;
scr |= CCSR_SSI_SCR_SYN;
}
@@ -1101,6 +1103,7 @@ static const struct snd_soc_component_driver 
fsl_ssi_component = {
 
 static struct snd_soc_dai_driver fsl_ssi_ac97_dai = {
.bus_control = true,
+   .probe = fsl_ssi_dai_probe,
.playback = {
.stream_name = AC97 Playback,
.channels_min = 2,
@@ -1127,10 +1130,17 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
struct regmap *regs = fsl_ac97_data-regs;
unsigned int lreg;
unsigned int lval;
+   int ret;
 
if (reg  0x7f)
return;
 
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 write clk_prepare_enable failed: %d\n,
+   ret);
+   return;
+   }
 
lreg = reg   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1141,6 +1151,8 @@ static void fsl_ssi_ac97_write(struct snd_ac97 *ac97, 
unsigned short reg,
regmap_update_bits(regs, CCSR_SSI_SACNT, CCSR_SSI_SACNT_RDWR_MASK,
CCSR_SSI_SACNT_WR);
udelay(100);
+
+   clk_disable_unprepare(fsl_ac97_data-clk);
 }
 
 static unsigned short fsl_ssi_ac97_read(struct snd_ac97 *ac97,
@@ -1151,6 +1163,14 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
unsigned short val = -1;
u32 reg_val;
unsigned int lreg;
+   int ret;
+
+   ret = clk_prepare_enable(fsl_ac97_data-clk);
+   if (ret) {
+   pr_err(ac97 read clk_prepare_enable failed: %d\n,
+   ret);
+   return -1;
+   }
 
lreg = (reg  0x7f)   12;
regmap_write(regs, CCSR_SSI_SACADD, lreg);
@@ -1162,6 +1182,8 @@ static unsigned short fsl_ssi_ac97_read(struct snd_ac97 
*ac97,
regmap_read(regs, CCSR_SSI_SACDAT, reg_val);
val = (reg_val  4)  0x;
 
+   clk_disable_unprepare(fsl_ac97_data-clk);
+
return val;
 }
 
@@ -1291,6 +1313,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+   bool newbinding;
 
of_id = of_match_device(fsl_ssi_ids, pdev-dev);
if (!of_id || !of_id-data)
@@ -1320,7 +1343,11 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
fsl_ac97_data = ssi_private;
 
-   snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   ret = snd_soc_set_ac97_ops_of_reset(fsl_ssi_ac97_ops, pdev);
+   if (ret) {
+   dev_err(pdev-dev, could not set AC'97 ops\n);
+   return ret;
+   }
} else {
/* Initialize this copy of the CPU DAI driver structure */
memcpy(ssi_private-cpu_dai_drv, fsl_ssi_dai_template,
@@ -1357,7 +1384,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
-   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+
ssi_private-cpu_dai_drv.symmetric_channels = 1;
ssi_private-cpu_dai_drv.symmetric_samplebits = 1;
}
@@ -1405,7 +1434,8 @@ static int fsl_ssi_probe(struct platform_device *pdev)
 * that the machine driver uses new binding which does 

Re: [PATCH] ASoC: fsl_ssi: fix AC'97 mode

[Maciej S. Szmigiero]

Hello Fabio,

W dniu 28.06.2015 01:06, Fabio Estevam pisze:
 Hi Maciej,
 
 On Sat, Jun 27, 2015 at 7:51 PM, Maciej S. Szmigiero
 m...@maciej.szmigiero.name wrote:
 Currently the AC'97 mode in fsl_ssi driver isn't functional.
 
 Thanks for the fix. I look forward to test it on my udoo board.

Thanks.

 This patch implements the following changes to make it work
 properly:
 * IPG clock have to be enabled during AC'97 CODEC
 register access,
 * AC'97 DAI driver struct need the same probe method as
 I2S one to setup DMA params,
 * AC'97 bus can support asymmetric playback/capture rates,
 * Check whether setting AC'97 ops succeeded and
 clean them on removal so the driver can be reloaded,
 * AC'97 CODEC will be instantiated in AC'97 mode,
 * Set DAI format function small fixes in AC'97 mode.
 
 It seems like a lot of changes in a single patch.
 
 Care to split it into smaller pieces?

OK, I will resend this split into individual patches.

 +
 +   ret = clk_prepare_enable(fsl_ac97_data-clk);
 +   if (ret) {
 +   pr_err(ac97 read clk_prepare_enable failed: %d\n,
 +   ret);
 +   return -1;

 'return ret' would be better here.

This function normal return value is an AC'97 register value,
so isn't more appropriate to return 0x in case of error
than linux error code?

 Thanks

Best regards,
Maciej Szmigiero

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev