Messages in this thread | | | Date | Thu, 26 Oct 2023 10:24:45 +0200 | Subject | Re: [PATCH] clk: mediatek: mt8186: Change I2C 4/5/6 ap clocks parent to infra | From | AngeloGioacchino Del Regno <> |
| |
Il 26/10/23 05:49, Yu-chang Lee (李禹璋) ha scritto: > On Wed, 2023-10-25 at 13:29 +0200, AngeloGioacchino Del Regno wrote: >> 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/ > > According to Hardware designer this arbiter clock is also lagecy of > previous design and serve no function. And they are conneted to top_i2c > as well. > > top_i2c-> CLK_INFRA_AO_I2C{1,2,5}_ARBITER > > Also may I know the experiment that lead to the conclusion that you > need the ARBITER clock, and the clock tree is incorrect? I will bring > it back to discuss with our I2C owner. >
I had lockups during boot and PM suspend/resume, plus, I was getting issues with losing trackpad functionality; adding the arbiter clocks fixed the issue.
Regards, Angelo
| |