Re: [PATCH] clk: mediatek: mt8186: Change I2C 4/5/6 ap clocks parent to infra

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 25/10/23 11:50, Yu-chang Lee (李禹璋) ha scritto:
> On Tue, 2023-10-24 at 17:20 +0800, Chen-Yu Tsai wrote:
>>   	
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   On Tue, Oct 24, 2023 at 3:47 PM Yu-chang Lee (李禹璋)
>> <Yu-chang.Lee@mediatek.com> wrote:
>>>
>>> On Tue, 2023-10-24 at 10:58 +0800, Chen-Yu Tsai wrote:
>>>> On Tue, Oct 24, 2023 at 10:52 AM Stephen Boyd <sboyd@kernel.org>
>>>> wrote:
>>>>>
>>>>> Quoting Chen-Yu Tsai (2023-10-19 22:06:35)
>>>>>> On Thu, Oct 19, 2023 at 8:49 PM AngeloGioacchino Del Regno
>>>>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>>>>
>>>>>>> Fix the parenting of clocks imp_iic_wrap_ap_clock_i2c{4-6},
>> as
>>>>>>> those
>>>>>>> are effectively parented to infra_ao_i2c{4-6} and not to
>> the
>>>>>>> I2C_AP.
>>>>>>> This permits the correct (and full) enablement and
>> disablement
>>>>>>> of the
>>>>>>> I2C4, I2C5 and I2C6 bus clocks, satisfying the whole clock
>> tree
>>>>>>> of
>>>>>>> those.
>>>>>>>
>>>>>>> As an example, when requesting to enable
>>>>>>> imp_iic_wrap_ap_clock_i2c4:
>>>>>>>
>>>>>>> Before: infra_ao_i2c_ap -> imp_iic_wrap_ap_clock_i2c4
>>>>>>> After:  infra_ao_i2c_ap -> infra_ao_i2c4 ->
>>>>>>> imp_iic_wrap_ap_clock_i2c4
>>>>>>>
>>>>>>> Fixes: 66cd0b4b0ce5 ("clk: mediatek: Add MT8186 imp i2c
>> wrapper
>>>>>>> clock support")
>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <
>>>>>>> angelogioacchino.delregno@collabora.com>
>>>>>>
>>>>>> I'm curious about what led to discovering this error?
>>>>>>
>>>>>
>>>>> Is that an acked-by?
>>>>
>>>> MediaTek engineers are saying the original code already matches
>> the
>>>> documentation provided by their hardware engineers. I'm trying to
>> get
>>>> them to respond on the mailing list.
>>>>
>>>> ChenYu
>>>>
>>> After checking with I2C clock hardware designer there is no
>>> infra_ao_i2c{4-6} clock gate in between. And the clock document at
>> hand
>>> aslo shows the same result. Generallly speaking, we would like to
>> keep
>>> sw setting align with the hardware design document. I would
>> recommand
>>> not to change this part of code, but enable infra_ao_i2c{4-6} prior
>> to
>>> the usage of imp_iic_wrap_ap_clock_i2c clock.
>>
>> Are infra_ao_i2c{4-6} actually used by the hardware? If so, for what
>> purpose?
> 
> According to hardware designer it servers no purpose. Just a legacy of
> previous design...
> 
>> If it is actually needed by the hardware and it is not in the
>> existing path,
>> then it needs to be described in the device tree and handled by the
>> driver.
>>
>> ChenYu
> 
> After reviewing hardware design diagram, hardware designer concludes
> that the clock tree is indeed
> 
> top_i2c -> infra_ao_i2c{4-6}
> top_i2c -> infra_ao_i2c_ap -> imp_iic_wrap_ap_clock_i2c{4-6}
> 
> so I think we should keep this clock relation unchanged.
> 
> Thanks
> YuChang
> 

Can you please also expand on CLK_INFRA_AO_I2C{1,2,5}_ARBITER clocks?
Is the I2C arbiter also legacy of previous designs?

Please check [1], as I've sent a commit adding those in the devicetree.

Thanks,
Angelo

[1]: 
https://lore.kernel.org/all/20231020075540.15191-1-angelogioacchino.delregno@collabora.com/