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

Commit Message

Nícolas F. R. A. Prado July 11, 2022, 8:57 p.m. UTC

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 <[e-mail]>

---

 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(-)

Comments

AngeloGioacchino Del Regno July 12, 2022, 10:41 a.m. UTC | #1

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 <[e-mail]>

For both MT8192 and MT8195:

Tested-by: AngeloGioacchino Del Regno <[e-mail]>

> ---
> 
>   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(-)
>

Chen-Yu Tsai July 12, 2022, 10:44 a.m. UTC | #2

Hi,

On Tue, Jul 12, 2022 at 4:57 AM Nícolas F. R. A. Prado
<[e-mail]> 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 <[e-mail]>
>
> ---
>
>  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 July 12, 2022, 10:55 a.m. UTC | #3

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
> <[e-mail]> 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 <[e-mail]>
>>
>> ---
>>
>>   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
>>

Chen-Yu Tsai July 12, 2022, 10:56 a.m. UTC | #4

On Tue, Jul 12, 2022 at 6:55 PM AngeloGioacchino Del Regno
<[e-mail]> 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
> > <[e-mail]> 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 <[e-mail]>
> >>
> >> ---
> >>
> >>   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 July 12, 2022, 10:57 a.m. UTC | #5

Il 12/07/22 12:56, Chen-Yu Tsai ha scritto:
> On Tue, Jul 12, 2022 at 6:55 PM AngeloGioacchino Del Regno
> <[e-mail]> 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
>>> <[e-mail]> 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 <[e-mail]>
>>>>
>>>> ---
>>>>
>>>>    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
>>>>
>>
>>

Nícolas F. R. A. Prado July 12, 2022, 2 p.m. UTC | #6

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
> > <[e-mail]> 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
> > > > <[e-mail]> 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!

Patch

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),