Messages in this thread | | | From | Chen-Yu Tsai <> | Date | Tue, 12 Jul 2022 18:56:46 +0800 | Subject | Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks |
| |
On Tue, Jul 12, 2022 at 6:55 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@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@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@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 > >> > >
| |