[PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks

[Chen-Yu Tsai]

On Tue, Jul 12, 2022 at 6:55 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno at collabora.com> wrote:
>
> Il 12/07/22 12:44, Chen-Yu Tsai ha scritto:
> > Hi,
> >
> > On Tue, Jul 12, 2022 at 4:57 AM Nícolas F. R. A. Prado
> > <nfraprado at collabora.com> wrote:
> >>
> >> The imp_iic_wrap clocks on mt8192/mt8195 require that the i2c_sel parent
> >> clock be enabled before their hardware status can be checked. Since this
> >> wasn't taken into account, reading from the clk_summary debugfs file
> >> would cause the system to completely freeze.
> >>
> >> Assuming that this clock is managed only by the kernel, and not by any
> >> firmware, simply drop the is_enabled() optional callback and instead
> >> rely on the enable count for the imp_iic_wrap clocks.
> >
> > That's the wrong way to go about it.
> >
> > The I2C clocks already have the CLK_OPS_PARENT_ENABLE flag set. So the
> > issue is that somewhere in the clk core, a piece of code is not honoring
> > that flag.
> >
> > And it seems that's in more than one place.
> >
>
> Uhm, you're right. I gave my Tested-by, but not a Reviewed-by because I
> wasn't really convinced about this solution being the best.
>
> Now that I think of it, the solution may be as simple as:
>
> clk.c
>
> static bool clk_core_is_enabled(struct clk_core *core)
> {
>         bool ret = false;
>
>         /*
>          * If this clock needs parent enabled, but its parent is
>          * off, we directly return false for two reasons:
>          * 1. This clock being enabled would be impossible
>          * 2. The platform may crash for unclocked access while
>          *    reading the status of this clock (where a .is_enabled
>          *    callback is provided).
>          */
>         if (core->flags & CLK_OPS_PARENT_ENABLE &&
>             !clk_core_is_enabled(core->parent))
>                 return false;
>
>         ... etc etc etc ...
> }
>
> Nícolas, did you try this approach?

I have a patch ready, but I got distracted by other stuff today.

ChenYu

> > Regards
> > ChenYu
> >
> >> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> >>
> >> ---
> >>
> >>   drivers/clk/mediatek/clk-gate.c                | 6 ++++++
> >>   drivers/clk/mediatek/clk-gate.h                | 1 +
> >>   drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c | 2 +-
> >>   drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c | 2 +-
> >>   4 files changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/clk/mediatek/clk-gate.c b/drivers/clk/mediatek/clk-gate.c
> >> index 421806236228..8e7c719a69b3 100644
> >> --- a/drivers/clk/mediatek/clk-gate.c
> >> +++ b/drivers/clk/mediatek/clk-gate.c
> >> @@ -124,6 +124,12 @@ static void mtk_cg_disable_inv_no_setclr(struct clk_hw *hw)
> >>          mtk_cg_clr_bit_no_setclr(hw);
> >>   }
> >>
> >> +const struct clk_ops mtk_clk_gate_ops_setclr_counted = {
> >> +       .enable         = mtk_cg_enable,
> >> +       .disable        = mtk_cg_disable,
> >> +};
> >> +EXPORT_SYMBOL_GPL(mtk_clk_gate_ops_setclr_counted);
> >> +
> >>   const struct clk_ops mtk_clk_gate_ops_setclr = {
> >>          .is_enabled     = mtk_cg_bit_is_cleared,
> >>          .enable         = mtk_cg_enable,
> >> diff --git a/drivers/clk/mediatek/clk-gate.h b/drivers/clk/mediatek/clk-gate.h
> >> index d9897ef53528..b5502b2911f5 100644
> >> --- a/drivers/clk/mediatek/clk-gate.h
> >> +++ b/drivers/clk/mediatek/clk-gate.h
> >> @@ -19,6 +19,7 @@ extern const struct clk_ops mtk_clk_gate_ops_setclr;
> >>   extern const struct clk_ops mtk_clk_gate_ops_setclr_inv;
> >>   extern const struct clk_ops mtk_clk_gate_ops_no_setclr;
> >>   extern const struct clk_ops mtk_clk_gate_ops_no_setclr_inv;
> >> +extern const struct clk_ops mtk_clk_gate_ops_setclr_counted;
> >>
> >>   struct mtk_gate_regs {
> >>          u32 sta_ofs;
> >> diff --git a/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c b/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
> >> index 700356ac6a58..900ee601169c 100644
> >> --- a/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
> >> +++ b/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
> >> @@ -20,7 +20,7 @@ static const struct mtk_gate_regs imp_iic_wrap_cg_regs = {
> >>
> >>   #define GATE_IMP_IIC_WRAP(_id, _name, _parent, _shift)                 \
> >>          GATE_MTK_FLAGS(_id, _name, _parent, &imp_iic_wrap_cg_regs, _shift,      \
> >> -               &mtk_clk_gate_ops_setclr, CLK_OPS_PARENT_ENABLE)
> >> +               &mtk_clk_gate_ops_setclr_counted, CLK_OPS_PARENT_ENABLE)
> >>
> >>   static const struct mtk_gate imp_iic_wrap_c_clks[] = {
> >>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_C_I2C10, "imp_iic_wrap_c_i2c10", "infra_i2c0", 0),
> >> diff --git a/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c b/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c
> >> index fbc809d05072..e50a77b844f4 100644
> >> --- a/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c
> >> +++ b/drivers/clk/mediatek/clk-mt8195-imp_iic_wrap.c
> >> @@ -18,7 +18,7 @@ static const struct mtk_gate_regs imp_iic_wrap_cg_regs = {
> >>
> >>   #define GATE_IMP_IIC_WRAP(_id, _name, _parent, _shift)                         \
> >>          GATE_MTK_FLAGS(_id, _name, _parent, &imp_iic_wrap_cg_regs, _shift,      \
> >> -               &mtk_clk_gate_ops_setclr, CLK_OPS_PARENT_ENABLE)
> >> +               &mtk_clk_gate_ops_setclr_counted, CLK_OPS_PARENT_ENABLE)
> >>
> >>   static const struct mtk_gate imp_iic_wrap_s_clks[] = {
> >>          GATE_IMP_IIC_WRAP(CLK_IMP_IIC_WRAP_S_I2C5, "imp_iic_wrap_s_i2c5", "top_i2c", 0),
> >> --
> >> 2.37.0
> >>
>
>