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

[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@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > > 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > > > > >
> > > > > > > > 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@xxxxxxxxxxxxx/
>
> 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