| Message ID | 20231019124914.13545-1-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: mediatek: mt8186: Change I2C 4/5/6 ap clocks parent to infra | expand |
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
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),
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(-)