Re: [PATCH v8 1/2] arm64: dts: imx8mp: Add SAI, SDMA, AudioMIX

[Marek Vasut <marex@denx.de>]

On 8/30/23 21:59, Adam Ford wrote:
> On Wed, Aug 30, 2023 at 2:10 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 8/30/23 04:44, Adam Ford wrote:
>>
>> Hi,
>>
>>> I have a question about the clocking for eASRC and PDM.
>>>
>>>> +
>>>> +                       audio_blk_ctrl: clock-controller@30e20000 {
>>>> +                               compatible = "fsl,imx8mp-audio-blk-ctrl";
>>>> +                               reg = <0x30e20000 0x10000>;
>>>> +                               #clock-cells = <1>;
>>>> +                               clocks = <&clk IMX8MP_CLK_AUDIO_ROOT>,
>>>> +                                        <&clk IMX8MP_CLK_SAI1>,
>>>> +                                        <&clk IMX8MP_CLK_SAI2>,
>>>> +                                        <&clk IMX8MP_CLK_SAI3>,
>>>> +                                        <&clk IMX8MP_CLK_SAI5>,
>>>> +                                        <&clk IMX8MP_CLK_SAI6>,
>>>> +                                        <&clk IMX8MP_CLK_SAI7>;
>>>> +                               clock-names = "ahb",
>>>> +                                             "sai1", "sai2", "sai3",
>>>> +                                             "sai5", "sai6", "sai7";
>>>> +                               power-domains = <&pgc_audio>;
>>>> +                       };
>>>> +               };
>>>> +
>>>
>>> I am trying to plumb in the micfil driver with a PDM microphone on a
>>> Plus.  I have SAI3 and SAI5 audio working, but if I try to use the
>>> micfil, the PDM clock doesn't get turned on, and the micfil doesn't
>>> appear to see anything coming in.  I was curious why the
>>> audio_blk_ctrl has clock entries for IMX8MP_CLK_SAIx, but there isn't
>>> one for the PDM nor the ASRC clocks.
>>
>> I only ever needed SAI, so that was what was tested on the EVK .
> 
> That makes sense.
> 
>>
>>> I added the MICFIL noted to the
>>> 8mp in a previous patch [1], and I am trying to customize the MICFIL
>>> node as follows:
>>>
>>> &micfil {
>>> #sound-dai-cells = <0>;
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&pinctrl_pdm>;
>>> assigned-clocks = <&clk IMX8MP_CLK_PDM>;
>>> assigned-clock-parents = <&clk IMX8MP_AUDIO_PLL1_OUT>;
>>> assigned-clock-rates = <196608000>;
>>> status = "okay";
>>> };
>>>
>>> I also noticed in the down-stream kernel, the pdm_ipg_clk and
>>> pdm_root_clk are shared gates with separate parents.
>>>
>>> The PDM tree of the down-stream kernel looks like this:
>>>    audio_pll1_ref_sel                0        0        0    24000000
>>>        0     0  50000         Y
>>>          audio_pll1                     0        0        0   393216000
>>>           0     0  50000         Y
>>>             audio_pll1_bypass           0        0        0   393216000
>>>           0     0  50000         Y
>>>                audio_pll1_out           0        0        0   393216000
>>>           0     0  50000         N
>>>                   pdm                   0        0        0   196608000
>>>           0     0  50000         N
>>>                      pdm_root           0        0        0   196608000
>>>           0     0  50000         N
>>>                         pdm_sel         0        0        0   196608000
>>>           0     0  50000         Y
>>>                            pdm_root_clk       0        0        0
>>> 196608000          0     0  50000         N
>>>
>>> The PDM tree of the mainline looks like this:
>>>
>>>      audio_pll1_ref_sel                0        0        0    24000000
>>>          0     0  50000         Y
>>>          audio_pll1                     0        0        0   393216000
>>>           0     0  50000         Y
>>>             audio_pll1_bypass           0        0        0   393216000
>>>           0     0  50000         Y
>>>                audio_pll1_out           0        0        0   393216000
>>>           0     0  50000         N
>>>                   pdm                   0        0        0   196608000
>>>           0     0  50000         N
>>>                      pdm_root           0        0        0   196608000
>>>           0     0  50000         N
>>>                         pdm_sel         0        0        0   196608000
>>>           0     0  50000         Y
>>>
>>> It seems like the "pdm_root_clk" generated by the shared audo-blk
>>> down-sream driver is missing from the mainline.  I looked up the clock
>>> I referenced when I attempted to enable the miffil, but
>>> 'IMX8MP_CLK_AUDIOMIX_PDM_ROOT doesn't appear to be configured in
>>> either clk-imx8mp.c or clk-imx8mp-audiomix.c.  Maybe it's obscured by
>>> the macros, but it seems like the pdm_sel should somehow have an
>>> additional variable for the shared clock and an additional clock like
>>> pdm_root_clk assigned with it.
>>>
>>> I have similar configurations for Mini and Nano, and both of them are
>>> able to record audio, so I think there might be a clock issue
>>> somewhere related to the audiomix driver, and not a misconfiguration
>>> of the sound-card or the micfil itself.
>>
>> Shouldn't the micfil be somehow a consumer of the pdm_sel clock , and
>> enable those clock in the driver ?
> 
> Micfil references IMX8MP_CLK_AUDIOMIX_PDM_IPG, and
> IMX8MP_CLK_AUDIOMIX_PDM_ROOT.  I am not convinced the
> IMX8MP_CLK_AUDIOMIX_PDM_ROOT exists beyond a #define in an include
> directory.  I tried making it use pdm_sel, but it threw an error.  I
> am not near my system, so I'm sorry I don't have more details.
> 
> In the downstream kernel IMX8MP_CLK_AUDIOMIX_PDM_ROOT was a child of
> pdm_sel, but I am not certain as to what the difference between them
> was since they appeared to be shared.

The pdm_sel is definitely a mux . Is there a follow-up gate after the mux ?