driver: imx: Add PDM MICFIL driver #8184

dbaluta · 2023-09-11T13:54:09Z

The Pulse Density Modulated Microphone Interface (MICFIL) is a popular way to deliver audio from microphones to the processor in several applications, such as mobile telephones. However, current digital-audio systems use multibit audio signal (also known as multibit PCM) to represent the signal. This block implements the required digital interface to provide a 24-bits audio signal from a PDM microphone bitstream in a configurable output sampling rate.

This patch adds initial support for PDM MICFIL IP found on i.MX8MP board.

Follow PRs will add:

topology for PDM MICFIL on imx8mp
support for multi fifo script

lgirdwood

@dbaluta I guess this also needs to land in Zephyr at some point ?

dbaluta · 2023-09-11T15:19:46Z

@dbaluta I guess this also needs to land in Zephyr at some point ?

Yes. Sometimes in Q1 2024.

lgirdwood · 2023-09-11T15:25:27Z

@iuliana-prodan good for you ?

lgirdwood · 2023-09-11T15:32:18Z

@LaurentiuM1234 good for you ?

kv2019i

Looks good!

src/drivers/imx/micfil.c

RanderWang

LGTM for most part

src/drivers/imx/micfil.c

dbaluta · 2023-09-12T13:07:08Z

@iuliana-prodan @kv2019i addressed all your comments.

@lyakh @lgirdwood it looks like there are some problems with Intel build with zephyr.

Care to have a look?

 In file included from /zep_workspace/zephyr/include/zephyr/toolchain.h:50,
                 from /zep_workspace/zephyr/include/zephyr/sys/time_units.h:10,
                 from /zep_workspace/zephyr/include/zephyr/sys/util.h:640,
                 from /zep_workspace/sof/src/include/sof/audio/module_adapter/iadk/adsp_stddef.h:15,
                 from /zep_workspace/sof/src/audio/module_adapter/iadk/module_initial_settings_concrete.cpp:7:
/zep_workspace/zephyr/include/zephyr/toolchain/gcc.h:87: error: "ZRESTRICT" redefined [-Werror]
   87 | #define ZRESTRICT __restrict
      | 
In file included from /zep_workspace/zephyr/include/zephyr/sys/util.h:18:
/zep_workspace/zephyr/include/zephyr/toolchain/common.h:33: note: this is the location of the previous definition
   33 | #define ZRESTRICT
      | 
cc1plus: all warnings being treated as errors
[252/361] Building C object modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/module_adapter/module/modules.c.obj
[253/361] Building C object modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/module_adapter/module_adapter.c.obj
ninja: build stopped: subcommand failed.

src/drivers/imx/micfil.c

lgirdwood · 2023-09-12T13:48:08Z

@iuliana-prodan @kv2019i addressed all your comments.

@lyakh @lgirdwood it looks like there are some problems with Intel build with zephyr.

Care to have a look?

 In file included from /zep_workspace/zephyr/include/zephyr/toolchain.h:50,
                 from /zep_workspace/zephyr/include/zephyr/sys/time_units.h:10,
                 from /zep_workspace/zephyr/include/zephyr/sys/util.h:640,
                 from /zep_workspace/sof/src/include/sof/audio/module_adapter/iadk/adsp_stddef.h:15,
                 from /zep_workspace/sof/src/audio/module_adapter/iadk/module_initial_settings_concrete.cpp:7:
/zep_workspace/zephyr/include/zephyr/toolchain/gcc.h:87: error: "ZRESTRICT" redefined [-Werror]
   87 | #define ZRESTRICT __restrict
      | 
In file included from /zep_workspace/zephyr/include/zephyr/sys/util.h:18:
/zep_workspace/zephyr/include/zephyr/toolchain/common.h:33: note: this is the location of the previous definition
   33 | #define ZRESTRICT
      | 
cc1plus: all warnings being treated as errors
[252/361] Building C object modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/module_adapter/module/modules.c.obj
[253/361] Building C object modules/sof/CMakeFiles/modules_sof.dir/zep_workspace/sof/src/audio/module_adapter/module_adapter.c.obj
ninja: build stopped: subcommand failed.

These are known about on the Zephyr side. IIUC, we are waiting on a Zephyr side fix. Not a blocker.

zephyr/CMakeLists.txt

lyakh · 2023-09-13T07:11:55Z

src/drivers/imx/micfil.c

+
+	switch (qsel) {
+	case MICFIL_QSEL_HIGH_QUALITY:
+		pdm_clk = rate * 8 * osr / 2; /* kfactor = 0.5 */


this is identical to the case below (8 / 2 = 4).

Yes, this has the same final value! But, I prefer to keep it that way to keep this in line with the documentation.

Oh... This is very... strange. It's your area so it's up to you to decide, but... This looks very confusing to me. You have multiple switch-case entries for different quality values, you seem to have different formulae, ant comments specify different "kfactor" values, but the result is the same. At the very least I'd put a comment there, preferably I'd unify all the cases and provide just one formula, and just explain in the comment how that matches the documentation

Yes, this comes from this table:

and this formula

I will add more comments to better explain the code.

What we want is to compute the CLK_DIV value so that we proper setup the MICFIL registers.

since

CLKDIV = MICFIL_CLK_ROOT / ( 2 * K * PDM_CLK_RATE)

and since

K and PDM_CLK_RATE have different values for different clk rates we end up with equal values sometimes, but as I said I prefer to write the complete formulas for documentation purposes.

yeah, an explanation would be good, because even with that table I don't understand: e.g. with high quality the formula in the table seems to be

pdm_clk = rate * 8 * osr;

whereas you use

pdm_clk = rate * 8 * osr / 2;

src/drivers/imx/micfil.c

src/ipc/ipc3/dai.c

CONFIG_SDMA_SCRIPT_CODE is always defined (even if not selected it defaults to empty string) so this is not a good way of including sdma script code header file. Make use of HAVE_SDMA_FIRMWARE boolean config in order to select the sdma script code header. Signed-off-by: Daniel Baluta <[email protected]>

The Pulse Density Modulated Microphone Interface (MICFIL) is a popular way to deliver audio from microphones to the processor in several applications, such as mobile telephones. However, current digital-audio systems use multibit audio signal (also known as multibit PCM) to represent the signal. This block implements the required digital interface to provide a 24-bits audio signal from a PDM microphone bitstream in a configurable output sampling rate. This patch adds initial support for PDM MICFIL IP found on i.MX8MP board. Signed-off-by: Daniel Baluta <[email protected]>

dbaluta · 2023-09-18T11:54:34Z

@lyakh all your comments should be addressed now

lyakh · 2023-09-18T13:37:44Z

@lyakh all your comments should be addressed now

What about #8184 (comment) - you're sure the calculations in the code are correct? I cannot see how they match the table that you provided

dbaluta · 2023-09-18T14:28:53Z

@lyakh all your comments should be addressed now

What about #8184 (comment) - you're sure the calculations in the code are correct? I cannot see how they match the table that you provided

The computation should be correct. Note that as specified in the comments the function micfil_get_pdm_clk returns pdm_clk * K.

I still would redo that switch-case statement: either it's wrong and should be fixed, or it's correct then I wouldn't use 3 different looking formulae producing the same result. But I cannot check correctness since I'm not working with this hardware and style is eventually subjective.

kv2019i · 2023-09-20T11:27:19Z

Feel free @dbaluta to merge

vijendarmukunda · 2023-10-26T12:57:35Z

It seems kernel side changes not merged yet. We are going to upstream few patches on SOF side adding new DAI configuration for AMD platforms. Could you please push kernel side changes?

dbaluta · 2023-10-26T13:38:47Z

@vijendarmukunda you are right, we are now in the middle of an internal release. Is it OK for you to delay this until next week?

vijendarmukunda · 2023-10-27T05:06:46Z

@dbaluta : Sounds Okay. We will wait.

dbaluta · 2023-10-27T12:09:27Z

@vijendarmukunda sent PR for review: thesofproject/linux#4670

dbaluta requested review from RanderWang, marcinszkudlinski, pblaszko, lgirdwood, plbossart, mmaka1, lbetlej and kv2019i as code owners September 11, 2023 13:54

dbaluta requested review from LaurentiuM1234 and iuliana-prodan September 11, 2023 14:18

lgirdwood approved these changes Sep 11, 2023

View reviewed changes

kv2019i approved these changes Sep 12, 2023

View reviewed changes

src/drivers/imx/micfil.c Outdated Show resolved Hide resolved

LaurentiuM1234 approved these changes Sep 12, 2023

View reviewed changes

dbaluta force-pushed the add_micfil branch from 30ae9c7 to 312db9b Compare September 12, 2023 07:55

RanderWang reviewed Sep 12, 2023

View reviewed changes

src/drivers/imx/micfil.c Show resolved Hide resolved

iuliana-prodan requested changes Sep 12, 2023

View reviewed changes

src/drivers/imx/micfil.c Outdated Show resolved Hide resolved

src/drivers/imx/micfil.c Outdated Show resolved Hide resolved

dbaluta force-pushed the add_micfil branch 3 times, most recently from 9d59c06 to efa2cb7 Compare September 12, 2023 12:49

dbaluta requested a review from marc-hb as a code owner September 12, 2023 12:49

iuliana-prodan reviewed Sep 12, 2023

View reviewed changes

src/drivers/imx/micfil.c Show resolved Hide resolved

iuliana-prodan self-requested a review September 12, 2023 13:48

dbaluta force-pushed the add_micfil branch from efa2cb7 to 23cb2a6 Compare September 12, 2023 14:04

iuliana-prodan approved these changes Sep 12, 2023

View reviewed changes

lyakh previously requested changes Sep 13, 2023

View reviewed changes

dbaluta force-pushed the add_micfil branch from 23cb2a6 to 6862e97 Compare September 13, 2023 15:28

dbaluta force-pushed the add_micfil branch from 6862e97 to d9aed2e Compare September 14, 2023 07:14

dbaluta merged commit 7ca9a2e into thesofproject:main Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: imx: Add PDM MICFIL driver #8184

driver: imx: Add PDM MICFIL driver #8184

dbaluta commented Sep 11, 2023

lgirdwood left a comment

dbaluta commented Sep 11, 2023

lgirdwood commented Sep 11, 2023

lgirdwood commented Sep 11, 2023

kv2019i left a comment

RanderWang left a comment

dbaluta commented Sep 12, 2023

lgirdwood commented Sep 12, 2023

lyakh Sep 13, 2023

dbaluta Sep 13, 2023

lyakh Sep 13, 2023

dbaluta Sep 13, 2023 •

edited

Loading

lyakh Sep 14, 2023

dbaluta commented Sep 18, 2023

lyakh commented Sep 18, 2023

dbaluta commented Sep 18, 2023

kv2019i commented Sep 20, 2023

vijendarmukunda commented Oct 26, 2023

dbaluta commented Oct 26, 2023

vijendarmukunda commented Oct 27, 2023

dbaluta commented Oct 27, 2023

driver: imx: Add PDM MICFIL driver #8184

driver: imx: Add PDM MICFIL driver #8184

Conversation

dbaluta commented Sep 11, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

dbaluta commented Sep 11, 2023

lgirdwood commented Sep 11, 2023

lgirdwood commented Sep 11, 2023

kv2019i left a comment

Choose a reason for hiding this comment

RanderWang left a comment

Choose a reason for hiding this comment

dbaluta commented Sep 12, 2023

lgirdwood commented Sep 12, 2023

lyakh Sep 13, 2023

Choose a reason for hiding this comment

dbaluta Sep 13, 2023

Choose a reason for hiding this comment

lyakh Sep 13, 2023

Choose a reason for hiding this comment

dbaluta Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

lyakh Sep 14, 2023

Choose a reason for hiding this comment

dbaluta commented Sep 18, 2023

lyakh commented Sep 18, 2023

dbaluta commented Sep 18, 2023

kv2019i commented Sep 20, 2023

vijendarmukunda commented Oct 26, 2023

dbaluta commented Oct 26, 2023

vijendarmukunda commented Oct 27, 2023

dbaluta commented Oct 27, 2023

dbaluta Sep 13, 2023 •

edited

Loading