Messages in this thread | | | Date | Wed, 25 Oct 2023 13:29:05 +0200 | Subject | Re: [PATCH] clk: mediatek: mt8186: Change I2C 4/5/6 ap clocks parent to infra | From | AngeloGioacchino Del Regno <> |
| |
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/
| |