clk: mediatek: mt8186: Change I2C 4/5/6 ap clocks parent to infra

  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>
---
 drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c | 6 +++---
 drivers/clk/mediatek/clk-mt8186-infra_ao.c     | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)
  

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?

ChenYu

> ---
>  drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c | 6 +++---
>  drivers/clk/mediatek/clk-mt8186-infra_ao.c     | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c b/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c
> index 640ccb553274..871b8ff4c287 100644
> --- a/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c
> +++ b/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c
> @@ -29,11 +29,11 @@ static const struct mtk_gate imp_iic_wrap_clks[] = {
>         GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C3,
>                 "imp_iic_wrap_ap_clock_i2c3", "infra_ao_i2c_ap", 3),
>         GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C4,
> -               "imp_iic_wrap_ap_clock_i2c4", "infra_ao_i2c_ap", 4),
> +               "imp_iic_wrap_ap_clock_i2c4", "infra_ao_i2c4", 4),
>         GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C5,
> -               "imp_iic_wrap_ap_clock_i2c5", "infra_ao_i2c_ap", 5),
> +               "imp_iic_wrap_ap_clock_i2c5", "infra_ao_i2c5", 5),
>         GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C6,
> -               "imp_iic_wrap_ap_clock_i2c6", "infra_ao_i2c_ap", 6),
> +               "imp_iic_wrap_ap_clock_i2c6", "infra_ao_i2c6", 6),
>         GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C7,
>                 "imp_iic_wrap_ap_clock_i2c7", "infra_ao_i2c_ap", 7),
>         GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C8,
> diff --git a/drivers/clk/mediatek/clk-mt8186-infra_ao.c b/drivers/clk/mediatek/clk-mt8186-infra_ao.c
> index 837304cd0ed7..c490f1a310f8 100644
> --- a/drivers/clk/mediatek/clk-mt8186-infra_ao.c
> +++ b/drivers/clk/mediatek/clk-mt8186-infra_ao.c
> @@ -132,7 +132,7 @@ static const struct mtk_gate infra_ao_clks[] = {
>         GATE_INFRA_AO2(CLK_INFRA_AO_AUDIO_26M_BCLK, "infra_ao_audio26m", "clk26m", 4),
>         GATE_INFRA_AO2(CLK_INFRA_AO_SSUSB_TOP_P1_HCLK, "infra_ao_ssusb_p1_hclk", "top_axi", 5),
>         GATE_INFRA_AO2(CLK_INFRA_AO_SPI1, "infra_ao_spi1", "top_spi", 6),
> -       GATE_INFRA_AO2(CLK_INFRA_AO_I2C4, "infra_ao_i2c4", "top_i2c", 7),
> +       GATE_INFRA_AO2(CLK_INFRA_AO_I2C4, "infra_ao_i2c4", "infra_ao_i2c_ap", 7),
>         GATE_INFRA_AO2(CLK_INFRA_AO_MODEM_TEMP_SHARE, "infra_ao_mdtemp", "clk26m", 8),
>         GATE_INFRA_AO2(CLK_INFRA_AO_SPI2, "infra_ao_spi2", "top_spi", 9),
>         GATE_INFRA_AO2(CLK_INFRA_AO_SPI3, "infra_ao_spi3", "top_spi", 10),
> @@ -145,7 +145,7 @@ static const struct mtk_gate infra_ao_clks[] = {
>         GATE_INFRA_AO2_FLAGS(CLK_INFRA_AO_SSPM, "infra_ao_sspm", "top_sspm", 15, CLK_IS_CRITICAL),
>         GATE_INFRA_AO2(CLK_INFRA_AO_SSUSB_TOP_P1_SYS,
>                        "infra_ao_ssusb_p1_sys", "top_ssusb_1p", 16),
> -       GATE_INFRA_AO2(CLK_INFRA_AO_I2C5, "infra_ao_i2c5", "top_i2c", 18),
> +       GATE_INFRA_AO2(CLK_INFRA_AO_I2C5, "infra_ao_i2c5", "infra_ao_i2c_ap", 18),
>         GATE_INFRA_AO2(CLK_INFRA_AO_I2C5_ARBITER, "infra_ao_i2c5a", "top_i2c", 19),
>         GATE_INFRA_AO2(CLK_INFRA_AO_I2C5_IMM, "infra_ao_i2c5_imm", "top_i2c", 20),
>         GATE_INFRA_AO2(CLK_INFRA_AO_I2C1_ARBITER, "infra_ao_i2c1a", "top_i2c", 21),
> @@ -167,7 +167,7 @@ static const struct mtk_gate infra_ao_clks[] = {
>                              CLK_IS_CRITICAL),
>         GATE_INFRA_AO3_FLAGS(CLK_INFRA_AO_SSPM_32K_SELF, "infra_ao_sspm_32k", "clk32k", 4,
>                              CLK_IS_CRITICAL),
> -       GATE_INFRA_AO3(CLK_INFRA_AO_I2C6, "infra_ao_i2c6", "top_i2c", 6),
> +       GATE_INFRA_AO3(CLK_INFRA_AO_I2C6, "infra_ao_i2c6", "infra_ao_i2c_ap", 6),
>         GATE_INFRA_AO3(CLK_INFRA_AO_AP_MSDC0, "infra_ao_ap_msdc0", "top_axi", 7),
>         GATE_INFRA_AO3(CLK_INFRA_AO_MD_MSDC0, "infra_ao_md_msdc0", "top_axi", 8),
>         GATE_INFRA_AO3(CLK_INFRA_AO_MSDC0_SRC, "infra_ao_msdc0_clk", "top_msdc50_0", 9),
> --
> 2.42.0
>
  

Il 20/10/23 07:06, Chen-Yu Tsai ha scritto:
> 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?
> 

I had lockups during boot when probing I2C, so some research led me to
discover that the clock tree wasn't fully satisfied... :-)

Cheers,
Angelo

> ChenYu
> 
>> ---
>>   drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c | 6 +++---
>>   drivers/clk/mediatek/clk-mt8186-infra_ao.c     | 6 +++---
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c b/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c
>> index 640ccb553274..871b8ff4c287 100644
>> --- a/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c
>> +++ b/drivers/clk/mediatek/clk-mt8186-imp_iic_wrap.c
>> @@ -29,11 +29,11 @@ static const struct mtk_gate imp_iic_wrap_clks[] = {
>>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C3,
>>                  "imp_iic_wrap_ap_clock_i2c3", "infra_ao_i2c_ap", 3),
>>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C4,
>> -               "imp_iic_wrap_ap_clock_i2c4", "infra_ao_i2c_ap", 4),
>> +               "imp_iic_wrap_ap_clock_i2c4", "infra_ao_i2c4", 4),
>>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C5,
>> -               "imp_iic_wrap_ap_clock_i2c5", "infra_ao_i2c_ap", 5),
>> +               "imp_iic_wrap_ap_clock_i2c5", "infra_ao_i2c5", 5),
>>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C6,
>> -               "imp_iic_wrap_ap_clock_i2c6", "infra_ao_i2c_ap", 6),
>> +               "imp_iic_wrap_ap_clock_i2c6", "infra_ao_i2c6", 6),
>>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C7,
>>                  "imp_iic_wrap_ap_clock_i2c7", "infra_ao_i2c_ap", 7),
>>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_AP_CLOCK_I2C8,
>> diff --git a/drivers/clk/mediatek/clk-mt8186-infra_ao.c b/drivers/clk/mediatek/clk-mt8186-infra_ao.c
>> index 837304cd0ed7..c490f1a310f8 100644
>> --- a/drivers/clk/mediatek/clk-mt8186-infra_ao.c
>> +++ b/drivers/clk/mediatek/clk-mt8186-infra_ao.c
>> @@ -132,7 +132,7 @@ static const struct mtk_gate infra_ao_clks[] = {
>>          GATE_INFRA_AO2(CLK_INFRA_AO_AUDIO_26M_BCLK, "infra_ao_audio26m", "clk26m", 4),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_SSUSB_TOP_P1_HCLK, "infra_ao_ssusb_p1_hclk", "top_axi", 5),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_SPI1, "infra_ao_spi1", "top_spi", 6),
>> -       GATE_INFRA_AO2(CLK_INFRA_AO_I2C4, "infra_ao_i2c4", "top_i2c", 7),
>> +       GATE_INFRA_AO2(CLK_INFRA_AO_I2C4, "infra_ao_i2c4", "infra_ao_i2c_ap", 7),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_MODEM_TEMP_SHARE, "infra_ao_mdtemp", "clk26m", 8),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_SPI2, "infra_ao_spi2", "top_spi", 9),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_SPI3, "infra_ao_spi3", "top_spi", 10),
>> @@ -145,7 +145,7 @@ static const struct mtk_gate infra_ao_clks[] = {
>>          GATE_INFRA_AO2_FLAGS(CLK_INFRA_AO_SSPM, "infra_ao_sspm", "top_sspm", 15, CLK_IS_CRITICAL),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_SSUSB_TOP_P1_SYS,
>>                         "infra_ao_ssusb_p1_sys", "top_ssusb_1p", 16),
>> -       GATE_INFRA_AO2(CLK_INFRA_AO_I2C5, "infra_ao_i2c5", "top_i2c", 18),
>> +       GATE_INFRA_AO2(CLK_INFRA_AO_I2C5, "infra_ao_i2c5", "infra_ao_i2c_ap", 18),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_I2C5_ARBITER, "infra_ao_i2c5a", "top_i2c", 19),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_I2C5_IMM, "infra_ao_i2c5_imm", "top_i2c", 20),
>>          GATE_INFRA_AO2(CLK_INFRA_AO_I2C1_ARBITER, "infra_ao_i2c1a", "top_i2c", 21),
>> @@ -167,7 +167,7 @@ static const struct mtk_gate infra_ao_clks[] = {
>>                               CLK_IS_CRITICAL),
>>          GATE_INFRA_AO3_FLAGS(CLK_INFRA_AO_SSPM_32K_SELF, "infra_ao_sspm_32k", "clk32k", 4,
>>                               CLK_IS_CRITICAL),
>> -       GATE_INFRA_AO3(CLK_INFRA_AO_I2C6, "infra_ao_i2c6", "top_i2c", 6),
>> +       GATE_INFRA_AO3(CLK_INFRA_AO_I2C6, "infra_ao_i2c6", "infra_ao_i2c_ap", 6),
>>          GATE_INFRA_AO3(CLK_INFRA_AO_AP_MSDC0, "infra_ao_ap_msdc0", "top_axi", 7),
>>          GATE_INFRA_AO3(CLK_INFRA_AO_MD_MSDC0, "infra_ao_md_msdc0", "top_axi", 8),
>>          GATE_INFRA_AO3(CLK_INFRA_AO_MSDC0_SRC, "infra_ao_msdc0_clk", "top_msdc50_0", 9),
>> --
>> 2.42.0
>>
  

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?
  

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
  

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.

Best Regards,
YuChang
  

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?
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
  

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
  

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/
  

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. 

Thanks,
YuChang
  

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