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 <[email protected]>
---
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
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 <[email protected]>
For both MT8192 and MT8195:
Tested-by: AngeloGioacchino Del Regno <[email protected]>
> ---
>
> 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(-)
>
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
> <[email protected]> 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 <[email protected]>
>>
>> ---
>>
>> 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
>>
On Tue, Jul 12, 2022 at 6:55 PM AngeloGioacchino Del Regno
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> >>
> >> ---
> >>
> >> 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
> >>
>
>
Hi,
On Tue, Jul 12, 2022 at 4:57 AM Nícolas F. R. A. Prado
<[email protected]> 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 <[email protected]>
>
> ---
>
> 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
>
Il 12/07/22 12:56, Chen-Yu Tsai ha scritto:
> On Tue, Jul 12, 2022 at 6:55 PM AngeloGioacchino Del Regno
> <[email protected]> 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
>>> <[email protected]> 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 <[email protected]>
>>>>
>>>> ---
>>>>
>>>> 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
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
> > <[email protected]> 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
> > > > <[email protected]> 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!