rpi-sound cards and upstreaming

[msperl]

@clivem: let us discuss these sound card things here...

I have tried the following upstream, but the card is not recognized:

/ {
    pcm5102a_codec: pcm5102a_codec {
        #sound-dai-cells = <0>;
        compatible = "ti,pcm5102a";
        status = "okay";
    };

    sound {
        compatible = "simple-audio-card";
        simple-audio-card,format = "i2s";
        simple-audio-card,name = "pcm5102a";
        status = "okay";
        simple-audio-card,cpu {
            sound-dai = <&i2s>;
        };
        simple-audio-card,codec {
            sound-dai = <&pcm5102a_codec>;
        };
    };
};
/* for some reasons this is required - simple-sound-card complains otherwise */
&i2s {
    #sound-dai-cells = <0>;
};

If I can make this work we can take things further from here...

As for the extension: we can look into it if we know how to parametrise things...

[clivem]

I didn't need that sound-dai-cells=<0> for i2s def. I am using the simple-pcm5102a-dac overlay config that I showed in the gist....

https://gist.github.com/clivem/9d1da750f666dc1d1ede4df761079742

dtoverlay=simple-pcm5102a-dac,card-name="snd_rpi_hifiberry_dac"

asoc-simple-card soc:sound: New simple-card: snd_rpi_hifiberry_dac
asoc-simple-card soc:sound: Revert to legacy daifmt parsing
asoc-simple-card soc:sound:         name : bcm2835-i2s-pcm5102a-hifi
asoc-simple-card soc:sound:         format : 4001
asoc-simple-card soc:sound:         cpu : bcm2835-i2s / 0
asoc-simple-card soc:sound:         codec : pcm5102a-hifi / 0
asoc-simple-card soc:sound: pcm5102a-hifi <-> 20203000.i2s mapping ok

[msperl]

At least upstream 4.6 requires it - or you get:

[   70.787180] [591] asoc-simple-card sound: New simple-card: HifiBerry DAC
[   70.787255] [591] asoc-simple-card sound: Revert to legacy daifmt parsing
[   70.787316] /sound/simple-audio-card,cpu: could not get #sound-dai-cells for /soc/i2s@7e203000
[   70.787342] asoc-simple-card sound: parse error -22
[   70.787401] asoc-simple-card: probe of sound failed with error -22

with this defined the driver loads...

[   60.427500] [589] asoc-simple-card sound: New simple-card: HifiBerry DAC
[   60.427584] [589] asoc-simple-card sound: Revert to legacy daifmt parsing
[   60.427701] [589] asoc-simple-card sound:    name : bcm2835-i2s-pcm5102a-hifi
[   60.427727] [589] asoc-simple-card sound:    format : 4001
[   60.427746] [589] asoc-simple-card sound:    cpu : bcm2835-i2s / 0
[   60.427765] [589] asoc-simple-card sound:    codec : pcm5102a-hifi / 0
[   60.429111] asoc-simple-card sound: pcm5102a-hifi <-> 20203000.i2s mapping ok

I still have no sound, but that is probably related to the *2 issue

[msperl]

So going further: the snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2) is not really needed, as that is the default set in i2s_bcm2835 if nothing is set.

So we are actually covered there...

This mostly leaves the custom bclk settings...

[clivem]

Sorry, not in front of computer at moment. ISTR only difference without card driver setting bclk ratio, is for 24 bit, where bclk_ratio from i2s driver will be 48. ie. 24 bits * 2 channels. Card driver uses physical width of bits, so S24_LE = 32 bits * 2 channels = 64.

hifiberry_dac card driver will request bclk_ratio of 32 for 16 bit and 64 for 24 and 32 bit. I2S driver will use 32 for 16 bit, 48 for 24 bit, and 64 for 32 bit.

[msperl]

here an example that - i guess - I got to work:

        hw-params-rule@0 {
            match@0 {
                method = "asoc_generic_hw_params_match_sample_bits";
                value = <32>;
            };
            match@1 {
                method = "asoc_generic_hw_params_match_rate";
                value = <48000>;
            };
            match@2 {
                method = "asoc_generic_hw_params_match_channels";
                value = <2>;
            };
            action@0 {
                method = "asoc_generic_hw_params_set_fixed_bclk_size";
                value = <80>;
            };
        };

And then playing: speaker-test -c 2 -r 48000 -F S32_LE -f 440 -t sine I get the following pcm clock:

root@raspcm:~# cat /sys/kernel/debug/clk/pcm/regdump
ctl = 0x00000001
div = 0x00005000

so parent clock 1 = OSC and a divider of 5.

Here the debug messages:

[root@raspcm:~# speaker-test -c 2 -r 48000 -F S32_LE -f 440 -t sine -l 1
speaker-test 1.0.28

Playback device is default
Stream parameters are 48000Hz, S32_LE, 2 channels
Sine wave rate is 440.0000Hz
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 64 to 65536
Period size range from 32 to 32768
Using max buffer size 65536
Periods = 4
was set period_size = 16384
was set buffer_size = 65536
 0 - Front Left
 1 - Front Right
Time per period = 4.196302
root@raspcm:~# cat /sys/kernel/debug/clk/pcm/regdump ctl = 0x00000001
div = 0x00005000
root@raspcm:~# dmesg
[  113.390048] [623] asoc-simple-card sound: New simple-card: HifiBerry DAC
[  113.390138] [623] asoc-simple-card sound: Revert to legacy daifmt parsing
[  113.390264] [623] asoc-simple-card sound:    name : bcm2835-i2s-pcm5102a-hifi
[  113.390291] [623] asoc-simple-card sound:    format : 4001
[  113.390312] [623] asoc-simple-card sound:    cpu : bcm2835-i2s / 3840000
[  113.390333] [623] asoc-simple-card sound:    codec : pcm5102a-hifi / 0
[  113.390380] [623] asoc-simple-card sound:    adding Rule: /sound/hw-params-rule@0
[  113.390454] [623] asoc-simple-card sound:        added match: /sound/hw-params-rule@0/match@0 - asoc_generic_hw_params_match_sample_bits [snd_soc_simple_card](00000020)
[  113.390512] [623] asoc-simple-card sound:        added match: /sound/hw-params-rule@0/match@1 - asoc_generic_hw_params_match_rate [snd_soc_simple_card](0000bb80)
[  113.390585] [623] asoc-simple-card sound:        added match: /sound/hw-params-rule@0/match@2 - asoc_generic_hw_params_match_channels [snd_soc_simple_card](00000002)
[  113.390655] [623] asoc-simple-card sound:        added action: /sound/hw-params-rule@0/action@0 - asoc_generic_hw_params_set_fixed_bclk_size [snd_soc_simple_card](00000050)
[  113.391991] asoc-simple-card sound: pcm5102a-hifi <-> 20203000.i2s mapping ok
--- here the speaker test ---
[  119.048124] [627]  bcm2835-i2s-pcm5102a-hifi: Trying to apply rule: /sound/hw-params-rule@0
[  119.048217] [627]  bcm2835-i2s-pcm5102a-hifi:    Running match asoc_generic_hw_params_match_sample_bits [snd_soc_simple_card](00000020)
[  119.048257] [627]  bcm2835-i2s-pcm5102a-hifi:    Running match asoc_generic_hw_params_match_rate [snd_soc_simple_card](0000bb80)
[  119.048309] [627]  bcm2835-i2s-pcm5102a-hifi:    Running match asoc_generic_hw_params_match_channels [snd_soc_simple_card](00000002)
[  119.048345] [627]  bcm2835-i2s-pcm5102a-hifi:    Running action asoc_generic_hw_params_set_fixed_bclk_size [snd_soc_simple_card](00000050)

So now in principle you can define (almost) any choice.

The way it is written this can easily get applied to any driver:

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 2389ab4..6b95ea1 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -8,6 +8,7 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+#include "hw-params-rules.h"
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/gpio.h>
@@ -33,6 +34,7 @@ struct simple_card_data {
        int gpio_hp_det_invert;
        int gpio_mic_det;
        int gpio_mic_det_invert;
+       struct list_head hw_params_rules;
        struct snd_soc_dai_link dai_link[];     /* dynamically allocated */
 };

@@ -51,7 +53,7 @@ static int asoc_simple_card_startup(struct snd_pcm_substream *substream)
        ret = clk_prepare_enable(dai_props->cpu_dai.clk);
        if (ret)
                return ret;
-
+
        ret = clk_prepare_enable(dai_props->codec_dai.clk);
        if (ret)
                clk_disable_unprepare(dai_props->cpu_dai.clk);
@@ -99,7 +101,9 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream,
                if (ret && ret != -ENOTSUPP)
                        goto err;
        }
-       return 0;
+
+       return asoc_generic_hw_params_process_rules(
+               &priv->hw_params_rules, substream, params);
 err:
        return ret;
 }
@@ -509,7 +513,8 @@ static int asoc_simple_card_parse_of(struct device_node *node,
        if (!priv->snd_card.name)
                priv->snd_card.name = priv->snd_card.dai_link->name;

-       return 0;
+       return asoc_generic_hw_params_rules_parse_of(
+               dev, node, &priv->hw_params_rules);
 }

 /* Decrease the reference count of the device nodes */

[msperl]

If you are interested look at my approach at:

• https://github.com/msperl/linux-upstream/commit/e9d87a938667ae20a27ec7df6b3ac43484fbc9e5 basic framework
• https://github.com/msperl/linux-upstream/commit/9887b23d541dffa3792f8281a09f54a4150d0d8a extending it to simple-sound

[clivem]

If you are interested look at my approach at:

Yes, more than interested, but at the moment I'm having to deal with a security breach issue, which means I won't get around to looking until this evening.

[msperl]

extensions for other "matches" or "actions" can get easily get added.

I really would like to use kallsyms_lookup_name, but that comes with some implications and could get abused, so it is not included yet, but the static mapping is used.

With that one could also add those "choose clock" things of the pcm51xx into that driver and use it with simple-config...

[clivem]

@msperl Martin, just thinking out loud, because I haven't even looked closely yet, are the match values, single values, or can be a list? Otherwise will have to have a hw-params-rule for every single sample rate at every single bit depth we want to cover. I'm thinking, something like this would result in the least lines of config needed in dts.....

        hw-params-rule@0 {
                match@0 {
                        method = "asoc_generic_hw_params_match_sample_bits";
                        value = <16>;
                };
                match@1 {
                        method = "asoc_generic_hw_params_match_rate";
                        value = <8000 16000 32000 48000 64000 96000>;
                };
                match@2 {
                        method = "asoc_generic_hw_params_match_channels";
                        value = <2>;
                };
                action@0 {
                        method = "asoc_generic_hw_params_set_fixed_bclk_size";
                        value = <50>;
                };
        };
        hw-params-rule@1 {
                match@0 {
                        method = "asoc_generic_hw_params_match_sample_bits";
                        value = <24 32>;
                };
                match@1 {
                        method = "asoc_generic_hw_params_match_rate";
                        value = <8000 16000 32000 48000 64000 96000>;
                };
                match@2 {
                        method = "asoc_generic_hw_params_match_channels";
                        value = <2>;
                };
                action@0 {
                        method = "asoc_generic_hw_params_set_fixed_bclk_size";
                        value = <100>;
                };
        };

[clivem]

@msperl When you tested the of parsing code, did you have more than a single rule in your dt config????

msperl.txt

[msperl]

@clivem I have only implemented single value matchers for now - an array is certainly feasible... Afair I have tested 2 rules at some point...

[clivem]

Afair I have tested 2 rules at some point...

This doesn't look right to me. It looks like matches/actions for rules 0,1,2 and 3 are being added to the list for rule 3.

Rule: /soc/sound/hw-params-rule@3
 added match: /soc/sound/hw-params-rule@3/match@2
 added match: /soc/sound/hw-params-rule@3/match@1
 added match: /soc/sound/hw-params-rule@3/match@0
 added match: /soc/sound/hw-params-rule@2/match@2
 added match: /soc/sound/hw-params-rule@2/match@1
 added match: /soc/sound/hw-params-rule@2/match@0
 added match: /soc/sound/hw-params-rule@1/match@2
 added match: /soc/sound/hw-params-rule@1/match@1
 added match: /soc/sound/hw-params-rule@1/match@0
 added match: /soc/sound/hw-params-rule@0/match@2
 added match: /soc/sound/hw-params-rule@0/match@1
 added match: /soc/sound/hw-params-rule@0/match@0
 added action: /soc/sound/hw-params-rule@3/action@0
 added action: /soc/sound/hw-params-rule@2/action@0
 added action: /soc/sound/hw-params-rule@1/action@0
 added action: /soc/sound/hw-params-rule@0/action@0

[msperl]

i know - i guess I did not read the fine details when testing the first rule... Working on the values thing right now and I guess that of_find_node_by_name does not stay within one parent and so I have to write my own matcher (and I had issue when i wrote that initially)...

[clivem]

Working on the values thing right now

OK, great. I'll come back to this later today. I think if you can make those matches takes arrays of u32's rather than a single value, and the logic is that if any value in the array is matched...... and the of parsing gets a tweak, we have a winner..... The question is, will upstream take it, or is there going to be the usual sharp intake of breath, followed by a "why do you even need this".... LOL.

As far as this framework is concerned, leaving aside clock selection again for the moment.... I wonder if you can find a few mins to think about this, and how we could make this sort of need to initialise things work via the same sort of framework on top of simple-card, so something like this PR submitted yesterday could use simple-card and not require a dedicated machine/card driver.......

https://github.com/raspberrypi/linux/pull/1476#issuecomment-219436532

I still have concerns that "simple" is going to end up being "complicated".... But as @pelwell said yesterday, "Yes, once again simple-card proves to be slightly too simple to be useful."

[msperl]

well - as for initialization we could in principle use the same interface - just:

sound {
  init@0 { method = "..."; values = "..."; };
};

It just depends on what is needed - but actually, as 4.6 just went out, i want to try to get those clock changes in so I would like to focus on those...

I will send that initial "patch" for hw_param hopefully today...

[msperl]

pushed an update for array - note that value changes to values for those that expect an array

[clivem]

It just depends on what is needed - but actually, as 4.6 just went out, i want to try to get those clock changes in so I would like to focus on those...

Yes, you should do that. The 'core' clock stuff is much more important than this.

pushed an update for array - note that value changes to values for those that expect an array

Just making a coffee and then I'll pull the latest.

[msperl]

just wait until I pushed the last change to fix that bug...

[ 2117.352855] [824] asoc-simple-card sound: New simple-card: HifiBerry DAC
[ 2117.352944] [824] asoc-simple-card sound: Revert to legacy daifmt parsing
[ 2117.353077] [824] asoc-simple-card sound:    name : bcm2835-i2s-pcm5102a-hifi
[ 2117.353105] [824] asoc-simple-card sound:    format : 4001
[ 2117.353127] [824] asoc-simple-card sound:    cpu : bcm2835-i2s / 7680000
[ 2117.353162] [824] asoc-simple-card sound:    codec : pcm5102a-hifi / 0
[ 2117.353201] [824] asoc-simple-card sound:    adding Rule: /sound/hw-params-rule@0
[ 2117.353271] [824] asoc-simple-card sound:            added match: /sound/hw-params-rule@0/match@0 - asoc_generic_hw_params_match_sample_bits [snd_soc_hw_params_rules](dace4450)
[ 2117.353338] [824] asoc-simple-card sound:            added match: /sound/hw-params-rule@0/match@1 - asoc_generic_hw_params_match_rate [snd_soc_hw_params_rules](dace4910)
[ 2117.353386] [824] asoc-simple-card sound:            added match: /sound/hw-params-rule@0/match@2 - asoc_generic_hw_params_match_channels [snd_soc_hw_params_rules](dace4350)
[ 2117.353452] [824] asoc-simple-card sound:            added action: /sound/hw-params-rule@0/action@0 - asoc_generic_hw_params_set_fixed_bclk_size [snd_soc_hw_params_rules](00000050)
[ 2117.353487] [824] asoc-simple-card sound:    adding Rule: /sound/hw-params-rule@1
[ 2117.353550] [824] asoc-simple-card sound:            added match: /sound/hw-params-rule@1/match@0 - asoc_generic_hw_params_match_sample_bits [snd_soc_hw_params_rules](dacbce90)
[ 2117.353601] [824] asoc-simple-card sound:            added match: /sound/hw-params-rule@1/match@1 - asoc_generic_hw_params_match_rate [snd_soc_hw_params_rules](dacbcf10)
[ 2117.353648] [824] asoc-simple-card sound:            added match: /sound/hw-params-rule@1/match@2 - asoc_generic_hw_params_match_channels [snd_soc_hw_params_rules](dacbcc10)
[ 2117.353711] [824] asoc-simple-card sound:            added action: /sound/hw-params-rule@1/action@0 - asoc_generic_hw_params_set_fixed_bclk_size [snd_soc_hw_params_rules](00000050)
[ 2117.355056] asoc-simple-card sound: pcm5102a-hifi <-> 20203000.i2s mapping ok
[ 2119.388367] [830]  bcm2835-i2s-pcm5102a-hifi: Trying to apply rule: /sound/hw-params-rule@0
[ 2119.388454] [830]  bcm2835-i2s-pcm5102a-hifi:        Running match asoc_generic_hw_params_match_sample_bits [snd_soc_hw_params_rules](dace4450)
[ 2119.388490] [830]  bcm2835-i2s-pcm5102a-hifi:        Running match asoc_generic_hw_params_match_rate [snd_soc_hw_params_rules](dace4910)
[ 2119.388511] [830]  bcm2835-i2s-pcm5102a-hifi: Trying to apply rule: /sound/hw-params-rule@1
[ 2119.388555] [830]  bcm2835-i2s-pcm5102a-hifi:        Running match asoc_generic_hw_params_match_sample_bits [snd_soc_hw_params_rules](dacbce90)
[ 2119.388589] [830]  bcm2835-i2s-pcm5102a-hifi:        Running match asoc_generic_hw_params_match_rate [snd_soc_hw_params_rules](dacbcf10)
[ 2119.388619] [830]  bcm2835-i2s-pcm5102a-hifi:        Running match asoc_generic_hw_params_match_channels [snd_soc_hw_params_rules](dacbcc10)
[ 2119.388648] [830]  bcm2835-i2s-pcm5102a-hifi:        Running action asoc_generic_hw_params_set_fixed_bclk_size [snd_soc_hw_params_rules](00000050)

Had to apply one more fix before fixing the dt-parsing...

Pushed and enjoy...

[clivem]

Thanks. I'll stop my current build and pull the latest.....

[clivem]

--- sound/soc/generic/hw-params-rules.c~    2016-05-17 13:51:52.000000000 +0100
+++ sound/soc/generic/hw-params-rules.c 2016-05-17 13:53:00.704744902 +0100
@@ -40,7 +40,7 @@
 static int asoc_generic_hw_params_read_u32array(
    struct device *dev, struct device_node *node, void **data)
 {
-   int i, size, ret;
+   int size, ret;
    struct snd_soc_size_u32array *array;

    size = of_property_count_elems_of_size(node, "values", sizeof(u32));
@@ -59,11 +59,9 @@

    array->size = size;

-   for (i = 0; i < size; i++) {
-       ret = of_property_read_u32(node, "values", &array->data[i]);
-       if (ret)
-           return ret;
-   }
+   ret = of_property_read_u32_array(node, "values", &array->data[0], size);
+   if (ret)
+       return ret;

    return 0;
 }

[msperl]

yes - there is that (but no of_for_each_child_with_name...) Will fix that.

Except for that: does it work as expected?

[clivem]

Except for that: does it work as expected?

No, don't submit it yet. At the moment I'm dealing with a security issue, which I need to concentrate on.

[msperl]

as a note: the new audioinjector-pi-soundcard that came into the foundation kernel would have - afaik - worked with the simple-card driver....

Or is there something missing?

[pelwell]

See https://github.com/raspberrypi/linux/pull/1476#issuecomment-219436532

[clivem]

Deja-vu......

[pelwell]

More like "Pas déjà lu".

[clivem]

@msperl Sorry, I have to go sort out a problem for my mother, so I need to cut and run without giving a good explanation of where I think we are now.... I'll explain about sorting the rules by priority and that we need a default rule with lowest priority to set the bclk back to default (or bcm2835-i2s driver controlled) after having had it set via a match.....

https://gist.github.com/clivem/ff1ac5a793524bdd066aea7f906e1bf6

Ignore the dupe rule@2 in the overlay. I screwed up the cut and paste. Don't have time to fix now.

[msperl]

actually just the "numbers" after hw_param_rule@... will define the order (but alpha-numerically, so 1, 10, 11, 19, 2, 20, 21, ... ,9, 90, 99, ...)

so if you need a lowest order rule use hw_param_rule@9999 or something like that

[msperl]

anyway - typically you would use reg = ; in such situations and not priority - at least as far as I understood...

[clivem]

@msperl Sorry Martin, another crazy day today......

Yes, I hear what you say about priority, but people (non devs) wont understand why @99 > @100. And I thought 'reg' was used for addresses, so I wouldn't have thought of using that as a descriptor for priority....

I have combined what I have been testing with (es9023, 384k and your hw_param_rule) on top of your upstream backport rpi-next-merged-patches-not-accepted-upstream dma/i2s, on top of downstream rpi-4.4.y HEAD.....

https://github.com/DigitalDreamtimeLtd/linux/commits/rpi-4.4.y-simple

For the ES9023 boards, I'm using/combining .... dtoverlay=simple-es9023-audio,384k,card-name="Akkordion" plus dtoverlay=simple-bclk-int-div-50-100 or dtoverlay=simple-bclk-int-div-40-80 For (HifiBerry 1st gen 'B' DAC and Pimoroni PHAT DAC on PiZero) pcm5102a .... dtoverlay=simple-pcm5102a-audio,card-name="snd_rpi_hifiberry_dac"

I know I'm probably going to get my ears chewed for match_noop. ;) Hope you don't have an issue with matchrate{lt,gt,div_by}.

[msperl]

So it is acceptable in principle? If so then I will submit it as an rfc patch as is.

The ordering could also try to parse strings to numbers, but then: what if someone uses hex instead of int?

As for the pcm5102: I would still use the compatible string or have a clock assigned >50 MHz to indicate the support of 384khz...

Martin

[clivem]

So it is acceptable in principle? If so then I will submit it as an rfc patch as is.

@msperl Martin, don't want to appear to be holding this back, but would you mind giving me this evening to collect my thoughts. Last 2 days have been crazy for me and I'm trying to catch-up with "real life" work related stuff...... It was as much as I could find time to do, pulling the patch sets into a single, downstream based, tree. (Which was mainly for a couple of guys who want to test, can do so without me needing to send them a 70 patch, patch set or having to point them at multiple trees.)

[msperl]

I would like to send something off tomorrow....

[clivem]

I would like to send something off tomorrow....

Understood. Don't hear back from me, do whatever you think is best. I'd like to keep the 'priority' field and it remain an integer. ;)

[msperl]

I have submitted it now with the "priority" property. (Alsa+dt mailing-lists)

[msperl]

let us see what they are complaining about and we fix that and when they are happy to accept it then we can take the next steps:

• dynamic method lookup
• init rules...

[clivem]

@msperl Martin, you have my attention again. Crazy week. last week. I'll go take a look at ALSA ML archive. Have you put the patches you submitted into a new branch?

[msperl]

No: I did not push it anywhere besides sending it to the mailing list...

Only comment so far is about using eBPF instead, which would make it much more complicated from the dev as well as user perspective...