Re: [PATCH v3 3/6] clk: imx: imx8mp: Add audiomix block control

[Marek Vasut <marex@denx.de>]

On 10/26/22 12:36, Shengjiu Wang wrote:
> Hi Marek
> 
> On Wed, Oct 26, 2022 at 5:10 AM Marek Vasut <marex@denx.de> wrote:
> 
>> On 10/20/22 05:06, Shengjiu Wang wrote:
>>> On Wed, Oct 19, 2022 at 10:33 PM Marek Vasut <marex@denx.de> wrote:
>>>
>>>> On 10/14/22 03:53, Shengjiu Wang wrote:
>>>>> Hi Marek
>>>>
>>>> Hi,
>>>>
>>>> [...]
>>>>
>>>>>> +static const struct clk_parent_data clk_imx8mp_audiomix_pll_parents[]
>>>> = {
>>>>>> +       { .fw_name = "osc_24m", .name = "osc_24m" },
>>>>>> +       { .name = "dummy" },
>>>>>> +       { .name = "dummy" },
>>>>>> +       { .name = "dummy" },
>>>>>> +};
>>>>>> +
>>>>>> +static const struct clk_parent_data
>>>> clk_imx8mp_audiomix_pll_bypass_sels[]
>>>>>> = {
>>>>>> +       { .fw_name = "sai_pll", .name = "sai_pll" },
>>>>>> +       { .fw_name = "sai_pll_ref_sel", .name = "sai_pll_ref_sel" },
>>>>>> +};
>>>>>> +
>>>>>> +#define CLK_GATE(gname, cname)
>>>>    \
>>>>>> +       {
>>>>    \
>>>>>> +               gname"_cg",
>>>>    \
>>>>>> +               IMX8MP_CLK_AUDIOMIX_##cname,
>>>> \
>>>>>> +               { .fw_name = "ahb", .name = "ahb" }, NULL, 1,
>>>>    \
>>>>>>
>>>>>> { .fw_name = "audio_root_clk", .name = "audio_root_clk" }, NULL, 1,
>>>>>>         \
>>>>>>
>>>>>> Should be the 'audio_root_clk' better?
>>>>>>
>>>>>> Then the 'clocks' and 'clock-names' can be removed in dts node?
>>>>>>
>>>>>> Will you continue to follow up this patch series?
>>>>
>>>> Sure. Did anyone from NXP finally test this patch series, and can
>>>> provide useful review ?
>>>>
>>>
>>> I have tested it, and I think "ahb" should be "audio_root_clk". others
>> LGTM.
>>
>> It seems those clock are actually called IMX8MP_CLK_AUDIO_AHB_ROOT in
>> the NXP downstream BSP, so those clock do drive AHB, correct ? If so, we
>> should keep the "ahb" name here, to differentiate them from already
>> existing IMX8MP_CLK_AUDIO_AXI , which seem to drive the AXI part.
>>
> 
> Seems IMX8MP_CLK_AUDIO_ROOT needs to be changed to
> IMX8MP_CLK_AUDIO_AHB_ROOT in clk-imx8mp.c
> 
> and
> "ocrama"  parent clock is "audio_axi_root"
> "earc_phy"  parent clock is "sai_pll_out_div2"
> "dsp" parent clock is "audio_axi_root"
> "dspdbg" parent clock is "audio_axi_root"
> "audpll" parent clock is "osc_24m"
> 
> so I think it is better to include downstream change in this patch
> series:
> -       hws[IMX8MP_CLK_AUDIO_ROOT] = imx_clk_hw_gate4("audio_root_clk",
> "ipg_root", ccm_base + 0x4650, 0);
> +
> +       hws[IMX8MP_CLK_AUDIO_AHB_ROOT] =
> imx_clk_hw_gate2_shared2("audio_ahb_root", "audio_ahb", ccm_base + 0x4650,
> 0, &share_count_audio);
> +       hws[IMX8MP_CLK_AUDIO_AXI_ROOT] =
> imx_clk_hw_gate2_shared2("audio_axi_root", "audio_axi", ccm_base + 0x4650,
> 0, &share_count_audio);
> +       hws[IMX8MP_CLK_SAI1_ROOT] = imx_clk_hw_gate2_shared2("sai1_root",
> "sai1", ccm_base + 0x4650, 0, &share_count_audio);
> +       hws[IMX8MP_CLK_SAI2_ROOT] = imx_clk_hw_gate2_shared2("sai2_root",
> "sai2", ccm_base + 0x4650, 0, &share_count_audio);
> +       hws[IMX8MP_CLK_SAI3_ROOT] = imx_clk_hw_gate2_shared2("sai3_root",
> "sai3", ccm_base + 0x4650, 0, &share_count_audio);
> +       hws[IMX8MP_CLK_SAI5_ROOT] = imx_clk_hw_gate2_shared2("sai5_root",
> "sai5", ccm_base + 0x4650, 0, &share_count_audio);
> +       hws[IMX8MP_CLK_SAI6_ROOT] = imx_clk_hw_gate2_shared2("sai6_root",
> "sai6", ccm_base + 0x4650, 0, &share_count_audio);
> +       hws[IMX8MP_CLK_SAI7_ROOT] = imx_clk_hw_gate2_shared2("sai7_root",
> "sai7", ccm_base + 0x4650, 0, &share_count_audio);
> +       hws[IMX8MP_CLK_PDM_ROOT] = imx_clk_hw_gate2_shared2("pdm_root",
> "pdm", ccm_base + 0x4650, 0, &share_count_audio);
> 

Can you prepare such a clock rename patch and submit it ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel