* [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks @ 2022-07-11 20:57 Nícolas F. R. A. Prado 2022-07-12 10:41 ` AngeloGioacchino Del Regno 2022-07-12 10:44 ` Chen-Yu Tsai 0 siblings, 2 replies; 7+ messages in thread From: Nícolas F. R. A. Prado @ 2022-07-11 20:57 UTC (permalink / raw) To: Stephen Boyd Cc: kernel, AngeloGioacchino Del Regno, Nícolas F. R. A. Prado, Chen-Yu Tsai, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek 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. 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks 2022-07-11 20:57 [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks Nícolas F. R. A. Prado @ 2022-07-12 10:41 ` AngeloGioacchino Del Regno 2022-07-12 10:44 ` Chen-Yu Tsai 1 sibling, 0 replies; 7+ messages in thread From: AngeloGioacchino Del Regno @ 2022-07-12 10:41 UTC (permalink / raw) To: Nícolas F. R. A. Prado, Stephen Boyd Cc: kernel, Chen-Yu Tsai, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek Il 11/07/22 22:57, Nícolas F. R. A. Prado ha scritto: > 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. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> For both MT8192 and MT8195: Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@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(-) > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks 2022-07-11 20:57 [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks Nícolas F. R. A. Prado 2022-07-12 10:41 ` AngeloGioacchino Del Regno @ 2022-07-12 10:44 ` Chen-Yu Tsai 2022-07-12 10:55 ` AngeloGioacchino Del Regno 1 sibling, 1 reply; 7+ messages in thread From: Chen-Yu Tsai @ 2022-07-12 10:44 UTC (permalink / raw) To: Nícolas F. R. A. Prado Cc: Stephen Boyd, kernel, AngeloGioacchino Del Regno, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek 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. 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks 2022-07-12 10:44 ` Chen-Yu Tsai @ 2022-07-12 10:55 ` AngeloGioacchino Del Regno 2022-07-12 10:56 ` Chen-Yu Tsai 0 siblings, 1 reply; 7+ messages in thread From: AngeloGioacchino Del Regno @ 2022-07-12 10:55 UTC (permalink / raw) To: Chen-Yu Tsai, Nícolas F. R. A. Prado Cc: Stephen Boyd, kernel, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek 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? > 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 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks 2022-07-12 10:55 ` AngeloGioacchino Del Regno @ 2022-07-12 10:56 ` Chen-Yu Tsai 2022-07-12 10:57 ` AngeloGioacchino Del Regno 0 siblings, 1 reply; 7+ messages in thread From: Chen-Yu Tsai @ 2022-07-12 10:56 UTC (permalink / raw) To: AngeloGioacchino Del Regno Cc: Nícolas F. R. A. Prado, Stephen Boyd, kernel, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek 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 > >> > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks 2022-07-12 10:56 ` Chen-Yu Tsai @ 2022-07-12 10:57 ` AngeloGioacchino Del Regno 2022-07-12 14:00 ` Nícolas F. R. A. Prado 0 siblings, 1 reply; 7+ messages in thread From: AngeloGioacchino Del Regno @ 2022-07-12 10:57 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Nícolas F. R. A. Prado, Stephen Boyd, kernel, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek Il 12/07/22 12:56, Chen-Yu Tsai ha scritto: > 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. > Let's just wait for your patch then, seems like being the most sensible option. Cheers! > 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 >>>> >> >> -- AngeloGioacchino Del Regno Software Engineer Collabora Ltd. Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK Registered in England & Wales, no. 5513718 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks 2022-07-12 10:57 ` AngeloGioacchino Del Regno @ 2022-07-12 14:00 ` Nícolas F. R. A. Prado 0 siblings, 0 replies; 7+ messages in thread From: Nícolas F. R. A. Prado @ 2022-07-12 14:00 UTC (permalink / raw) To: AngeloGioacchino Del Regno Cc: Chen-Yu Tsai, Stephen Boyd, kernel, Chun-Jie Chen, Ikjoon Jang, Matthias Brugger, Michael Turquette, Miles Chen, Ran Jianping, Weiyi Lu, linux-arm-kernel, linux-clk, linux-kernel, linux-mediatek On Tue, Jul 12, 2022 at 12:57:47PM +0200, AngeloGioacchino Del Regno wrote: > Il 12/07/22 12:56, Chen-Yu Tsai ha scritto: > > 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? From reading the core clock code, it's mentioned that CLK_OPS_PARENT_ENABLE is used "during gate/ungate, set rate and re-parent", there's no mention of "checking the gate state", so I assumed this operation was intentionally not handled by this flag. That's why I went for this solution. But from the discussion sounds like the flag should indeed be caring about the is_enabled() operation as well, so let's go with Chen-Yu's patch. Thanks, Nícolas > > > > I have a patch ready, but I got distracted by other stuff today. > > > > Let's just wait for your patch then, seems like being the most sensible option. > > Cheers! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-07-12 14:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-11 20:57 [PATCH] clk: mediatek: Don't check HW status for mt8192/5's imp_iic_wrap clocks Nícolas F. R. A. Prado 2022-07-12 10:41 ` AngeloGioacchino Del Regno 2022-07-12 10:44 ` Chen-Yu Tsai 2022-07-12 10:55 ` AngeloGioacchino Del Regno 2022-07-12 10:56 ` Chen-Yu Tsai 2022-07-12 10:57 ` AngeloGioacchino Del Regno 2022-07-12 14:00 ` Nícolas F. R. A. Prado
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox