[PATCHv2 0/6] clk: ti: reset handling support fixes

[PATCHv2 0/6] clk: ti: reset handling support fixes

[Tero Kristo]

Hi,

This is v2 of the earlier series [1] targeting remoteproc / reset support for
OMAP SoCs. None of the earlier patches have been retained, mostly everything
is re-written. :)

Couple of notes about the individual patches:

#1: needed so that reset handling code can find clkctrl handles
#2: just to convert the code to look a bit neater with all the bit
    handling logic
#3: new TI SoC only API for checking standby state for clocks, needed
    for remoteproc idle status handling
#4: new TI SoC only API for syncing up status between reset + associated
    clock
#5/#6: add missing IVA clkctrl clock entries for omap5, this has been
    just missed before and is needed as IVA has reset lines

I know its already quite late for 5.4, but in theory these
could be picked up for it also. If not, pushing for 5.5 is fine.

-Tero

[1] https://www.spinics.net/lists/linux-clk/msg40060.html


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 1/6] clk: ti: clkctrl: fix setting up clkctrl clocks

[Tero Kristo]

Apply the proper register function for clkctrl clocks, so they get
registered under the clk_hw_omap list also. This allows checking their
type runtime.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clkctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
index 975995eea15c..a914df2e9e1b 100644
--- a/drivers/clk/ti/clkctrl.c
+++ b/drivers/clk/ti/clkctrl.c
@@ -622,7 +622,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 		init.ops = &omap4_clkctrl_clk_ops;
 		hw->hw.init = &init;
 
-		clk = ti_clk_register(NULL, &hw->hw, init.name);
+		clk = ti_clk_register_omap_hw(NULL, &hw->hw, init.name);
 		if (IS_ERR_OR_NULL(clk))
 			goto cleanup;
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 2/6] clk: ti: clkctrl: convert to use bit helper macros instead of bitops

[Tero Kristo]

This improves the readibility of the code slightly, and makes modifying
the flags bit simpler.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clkctrl.c | 8 ++++----
 include/linux/clk/ti.h   | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
index a914df2e9e1b..d904a9a7626a 100644
--- a/drivers/clk/ti/clkctrl.c
+++ b/drivers/clk/ti/clkctrl.c
@@ -24,7 +24,7 @@
 #include <linux/timekeeping.h>
 #include "clock.h"
 
-#define NO_IDLEST			0x1
+#define NO_IDLEST			0
 
 #define OMAP4_MODULEMODE_MASK		0x3
 
@@ -158,7 +158,7 @@ static int _omap4_clkctrl_clk_enable(struct clk_hw *hw)
 
 	ti_clk_ll_ops->clk_writel(val, &clk->enable_reg);
 
-	if (clk->flags & NO_IDLEST)
+	if (test_bit(NO_IDLEST, &clk->flags))
 		return 0;
 
 	/* Wait until module is enabled */
@@ -187,7 +187,7 @@ static void _omap4_clkctrl_clk_disable(struct clk_hw *hw)
 
 	ti_clk_ll_ops->clk_writel(val, &clk->enable_reg);
 
-	if (clk->flags & NO_IDLEST)
+	if (test_bit(NO_IDLEST, &clk->flags))
 		goto exit;
 
 	/* Wait until module is disabled */
@@ -596,7 +596,7 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 		if (reg_data->flags & CLKF_HW_SUP)
 			hw->enable_bit = MODULEMODE_HWCTRL;
 		if (reg_data->flags & CLKF_NO_IDLEST)
-			hw->flags |= NO_IDLEST;
+			set_bit(NO_IDLEST, &hw->flags);
 
 		if (reg_data->clkdm_name)
 			hw->clkdm_name = reg_data->clkdm_name;
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 1e8ef96555ce..bb2c5af9082a 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -153,7 +153,7 @@ struct clk_hw_omap {
 	u8			fixed_div;
 	struct clk_omap_reg	enable_reg;
 	u8			enable_bit;
-	u8			flags;
+	unsigned long		flags;
 	struct clk_omap_reg	clksel_reg;
 	struct dpll_data	*dpll_data;
 	const char		*clkdm_name;
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 3/6] clk: ti: clkctrl: add new exported API for checking standby info

[Tero Kristo]

Standby status is provided for certain clkctrl clocks to see if the
given module has entered standby or not. This is mostly needed by
remoteproc code to see if the remoteproc has entered standby and the clock
can be turned off safely.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clkctrl.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/clk/ti.h   |  1 +
 2 files changed, 34 insertions(+)

diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
index d904a9a7626a..e3e0a66a6ce2 100644
--- a/drivers/clk/ti/clkctrl.c
+++ b/drivers/clk/ti/clkctrl.c
@@ -34,6 +34,9 @@
 #define OMAP4_IDLEST_MASK		(0x3 << 16)
 #define OMAP4_IDLEST_SHIFT		16
 
+#define OMAP4_STBYST_MASK		BIT(18)
+#define OMAP4_STBYST_SHIFT		18
+
 #define CLKCTRL_IDLEST_FUNCTIONAL	0x0
 #define CLKCTRL_IDLEST_INTERFACE_IDLE	0x2
 #define CLKCTRL_IDLEST_DISABLED		0x3
@@ -647,3 +650,33 @@ static void __init _ti_omap4_clkctrl_setup(struct device_node *node)
 }
 CLK_OF_DECLARE(ti_omap4_clkctrl_clock, "ti,clkctrl",
 	       _ti_omap4_clkctrl_setup);
+
+/**
+ * ti_clk_is_in_standby - Check if clkctrl clock is in standby or not
+ * @clk: clock to check standby status for
+ *
+ * Finds whether the provided clock is in standby mode or not. Returns
+ * true if the provided clock is a clkctrl type clock and it is in standby,
+ * false otherwise.
+ */
+u32 ti_clk_is_in_standby(struct clk *clk)
+{
+	struct clk_hw *hw;
+	struct clk_hw_omap *hwclk;
+	u32 val;
+
+	hw = __clk_get_hw(clk);
+
+	if (!omap2_clk_is_hw_omap(hw))
+		return false;
+
+	hwclk = to_clk_hw_omap(hw);
+
+	val = ti_clk_ll_ops->clk_readl(&hwclk->enable_reg);
+
+	if (val & OMAP4_STBYST_MASK)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index bb2c5af9082a..3fb777f7103a 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -298,6 +298,7 @@ struct ti_clk_features {
 
 void ti_clk_setup_features(struct ti_clk_features *features);
 const struct ti_clk_features *ti_clk_get_features(void);
+u32 ti_clk_is_in_standby(struct clk *clk);
 int omap3_noncore_dpll_save_context(struct clk_hw *hw);
 void omap3_noncore_dpll_restore_context(struct clk_hw *hw);
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Tero Kristo]

When an IP has both a clkctrl clock being fed into it and has hardware
reset lines, the control for these must be synced against each other.
While disabling a clock, all the hardware reset lines must be asserted
at the same time, and while enabling, resets must be deasserted.
Otherwise, the IP module fails to transition from to idle/active. To
handle this situation properly, a callback is added for clkctrl clocks
which is used by the PRM reset driver to tell the status of the reset
lines. This info is then used to sync state.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clkctrl.c | 44 ++++++++++++++++++++++++++++++++++++++++
 include/linux/clk/ti.h   |  1 +
 2 files changed, 45 insertions(+)

diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
index e3e0a66a6ce2..47a0d1398c6f 100644
--- a/drivers/clk/ti/clkctrl.c
+++ b/drivers/clk/ti/clkctrl.c
@@ -25,6 +25,8 @@
 #include "clock.h"
 
 #define NO_IDLEST			0
+#define HAS_RESET			1
+#define RESET_ASSERTED			2
 
 #define OMAP4_MODULEMODE_MASK		0x3
 
@@ -164,6 +166,9 @@ static int _omap4_clkctrl_clk_enable(struct clk_hw *hw)
 	if (test_bit(NO_IDLEST, &clk->flags))
 		return 0;
 
+	if (test_bit(RESET_ASSERTED, &clk->flags))
+		return 0;
+
 	/* Wait until module is enabled */
 	while (!_omap4_is_ready(ti_clk_ll_ops->clk_readl(&clk->enable_reg))) {
 		if (_omap4_is_timeout(&timeout, OMAP4_MAX_MODULE_READY_TIME)) {
@@ -193,6 +198,10 @@ static void _omap4_clkctrl_clk_disable(struct clk_hw *hw)
 	if (test_bit(NO_IDLEST, &clk->flags))
 		goto exit;
 
+	if (test_bit(HAS_RESET, &clk->flags) &&
+	    !test_bit(RESET_ASSERTED, &clk->flags))
+		goto exit;
+
 	/* Wait until module is disabled */
 	while (!_omap4_is_idle(ti_clk_ll_ops->clk_readl(&clk->enable_reg))) {
 		if (_omap4_is_timeout(&timeout,
@@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
 	return false;
 }
 EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
+
+/**
+ * ti_clk_notify_resets - Notify the clock driver associated reset status
+ * @clk: clock to notify reset status for
+ * @asserted: true if all HW reset lines are asserted
+ *
+ * Some clkctrl clocks have associated resets for them which effectively
+ * prevent the clock to transition from/to idle if the reset state is not
+ * in sync. For the clock to transition to idle properly, all associated
+ * resets must be asserted, and to leave idle, vice versa. To provide the
+ * current reset status, the reset driver should issue this callback.
+ */
+void ti_clk_notify_resets(struct clk *clk, bool asserted)
+{
+	struct clk_hw *hw;
+	struct clk_hw_omap *hwclk;
+
+	if (!clk)
+		return;
+
+	hw = __clk_get_hw(clk);
+
+	if (!omap2_clk_is_hw_omap(hw))
+		return;
+
+	hwclk = to_clk_hw_omap(hw);
+
+	set_bit(HAS_RESET, &hwclk->flags);
+
+	if (asserted)
+		set_bit(RESET_ASSERTED, &hwclk->flags);
+	else
+		clear_bit(RESET_ASSERTED, &hwclk->flags);
+}
+EXPORT_SYMBOL_GPL(ti_clk_notify_resets);
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 3fb777f7103a..ae34e3f5cf7a 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -299,6 +299,7 @@ struct ti_clk_features {
 void ti_clk_setup_features(struct ti_clk_features *features);
 const struct ti_clk_features *ti_clk_get_features(void);
 u32 ti_clk_is_in_standby(struct clk *clk);
+void ti_clk_notify_resets(struct clk *clk, bool asserted);
 int omap3_noncore_dpll_save_context(struct clk_hw *hw);
 void omap3_noncore_dpll_restore_context(struct clk_hw *hw);
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 5/6] dt-bindings: clk: add omap5 iva clkctrl definitions

[Tero Kristo]

OMAP5 device contains an IVA subsystem (Image and Video Accelerator.)
IVA subsystem clkctrl definitions are currently missing, so add them.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 include/dt-bindings/clock/omap5.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/clock/omap5.h b/include/dt-bindings/clock/omap5.h
index f3283957f48d..eb6d2fbe78f9 100644
--- a/include/dt-bindings/clock/omap5.h
+++ b/include/dt-bindings/clock/omap5.h
@@ -86,6 +86,10 @@
 #define OMAP5_UART5_CLKCTRL	OMAP5_CLKCTRL_INDEX(0x170)
 #define OMAP5_UART6_CLKCTRL	OMAP5_CLKCTRL_INDEX(0x178)
 
+/* iva clocks */
+#define OMAP5_IVA_CLKCTRL	OMAP5_CLKCTRL_INDEX(0x20)
+#define OMAP5_SL2IF_CLKCTRL	OMAP5_CLKCTRL_INDEX(0x28)
+
 /* dss clocks */
 #define OMAP5_DSS_CORE_CLKCTRL	OMAP5_CLKCTRL_INDEX(0x20)
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCHv2 6/6] clk: ti: omap5: add IVA subsystem clkctrl data

[Tero Kristo]

Add clkctrl data for the IVA subsystem (Image and Video Accelerator.)

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/ti/clk-54xx.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/ti/clk-54xx.c b/drivers/clk/ti/clk-54xx.c
index dafef7e70ba8..f63871dac600 100644
--- a/drivers/clk/ti/clk-54xx.c
+++ b/drivers/clk/ti/clk-54xx.c
@@ -286,6 +286,12 @@ static const struct omap_clkctrl_reg_data omap5_l4per_clkctrl_regs[] __initconst
 	{ 0 },
 };
 
+static const struct omap_clkctrl_reg_data omap5_iva_clkctrl_regs[] __initconst = {
+	{ OMAP5_IVA_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_iva_h12x2_ck" },
+	{ OMAP5_SL2IF_CLKCTRL, NULL, CLKF_HW_SUP, "dpll_iva_h12x2_ck" },
+	{ 0 },
+};
+
 static const char * const omap5_dss_dss_clk_parents[] __initconst = {
 	"dpll_per_h12x2_ck",
 	NULL,
@@ -469,6 +475,7 @@ const struct omap_clkctrl_data omap5_clkctrl_data[] __initconst = {
 	{ 0x4a008d20, omap5_l4cfg_clkctrl_regs },
 	{ 0x4a008e20, omap5_l3instr_clkctrl_regs },
 	{ 0x4a009020, omap5_l4per_clkctrl_regs },
+	{ 0x4a009220, omap5_iva_clkctrl_regs },
 	{ 0x4a009420, omap5_dss_clkctrl_regs },
 	{ 0x4a009620, omap5_l3init_clkctrl_regs },
 	{ 0x4ae07920, omap5_wkupaon_clkctrl_regs },
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 0/6] clk: ti: reset handling support fixes

[Stephen Boyd]

Quoting Tero Kristo (2019-08-27 23:59:23)
> Hi,
> 
> This is v2 of the earlier series [1] targeting remoteproc / reset support for
> OMAP SoCs. None of the earlier patches have been retained, mostly everything
> is re-written. :)
> 
> Couple of notes about the individual patches:
> 
> #1: needed so that reset handling code can find clkctrl handles
> #2: just to convert the code to look a bit neater with all the bit
>     handling logic
> #3: new TI SoC only API for checking standby state for clocks, needed
>     for remoteproc idle status handling
> #4: new TI SoC only API for syncing up status between reset + associated
>     clock
> #5/#6: add missing IVA clkctrl clock entries for omap5, this has been
>     just missed before and is needed as IVA has reset lines
> 
> I know its already quite late for 5.4, but in theory these
> could be picked up for it also. If not, pushing for 5.5 is fine.

Its sort of late. I guess let me throw it into fixes and try to send off
one more PR to Linus early next week.

Re: [PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Stephen Boyd]

Quoting Tero Kristo (2019-08-27 23:59:27)
> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
> index e3e0a66a6ce2..47a0d1398c6f 100644
> --- a/drivers/clk/ti/clkctrl.c
> +++ b/drivers/clk/ti/clkctrl.c
> @@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
>         return false;
>  }
>  EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
> +
> +/**
> + * ti_clk_notify_resets - Notify the clock driver associated reset status

This is completely unused in this patch series. What's going on?

> + * @clk: clock to notify reset status for
> + * @asserted: true if all HW reset lines are asserted
> + *
> + * Some clkctrl clocks have associated resets for them which effectively
> + * prevent the clock to transition from/to idle if the reset state is not
> + * in sync. For the clock to transition to idle properly, all associated
> + * resets must be asserted, and to leave idle, vice versa. To provide the
> + * current reset status, the reset driver should issue this callback.
> + */

Re: [PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Tero Kristo]

On 29/08/2019 23:05, Stephen Boyd wrote:
> Quoting Tero Kristo (2019-08-27 23:59:27)
>> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
>> index e3e0a66a6ce2..47a0d1398c6f 100644
>> --- a/drivers/clk/ti/clkctrl.c
>> +++ b/drivers/clk/ti/clkctrl.c
>> @@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
>>          return false;
>>   }
>>   EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
>> +
>> +/**
>> + * ti_clk_notify_resets - Notify the clock driver associated reset status
> 
> This is completely unused in this patch series. What's going on?

This is needed by the OMAP reset driver. See:

https://lwn.net/Articles/797597/

-Tero

>> + * @clk: clock to notify reset status for
>> + * @asserted: true if all HW reset lines are asserted
>> + *
>> + * Some clkctrl clocks have associated resets for them which effectively
>> + * prevent the clock to transition from/to idle if the reset state is not
>> + * in sync. For the clock to transition to idle properly, all associated
>> + * resets must be asserted, and to leave idle, vice versa. To provide the
>> + * current reset status, the reset driver should issue this callback.
>> + */

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Stephen Boyd]

Quoting Tero Kristo (2019-08-29 23:06:41)
> On 29/08/2019 23:05, Stephen Boyd wrote:
> > Quoting Tero Kristo (2019-08-27 23:59:27)
> >> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
> >> index e3e0a66a6ce2..47a0d1398c6f 100644
> >> --- a/drivers/clk/ti/clkctrl.c
> >> +++ b/drivers/clk/ti/clkctrl.c
> >> @@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
> >>          return false;
> >>   }
> >>   EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
> >> +
> >> +/**
> >> + * ti_clk_notify_resets - Notify the clock driver associated reset status
> > 
> > This is completely unused in this patch series. What's going on?
> 
> This is needed by the OMAP reset driver. See:
> 
> https://lwn.net/Articles/797597/
> 

Ok. I decided to punt this topic forward to next release at the least.
To clarify, TI is not special with regards to coordinating resets and
clk enable/disable state. Every other silicon vendor has the same
requirements and nobody is doing a good job at it.

Please devise a way that avoids making a tight coupling between the clk
driver and the reset driver in this way. Are the two in the same
register space? Perhaps we need to combine the two drivers then. Or can
this be implemented as a genpd that coordinates the resets and clk
controls for various devices?

Re: [PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Tero Kristo]

On 06/09/2019 19:15, Stephen Boyd wrote:
> Quoting Tero Kristo (2019-08-29 23:06:41)
>> On 29/08/2019 23:05, Stephen Boyd wrote:
>>> Quoting Tero Kristo (2019-08-27 23:59:27)
>>>> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
>>>> index e3e0a66a6ce2..47a0d1398c6f 100644
>>>> --- a/drivers/clk/ti/clkctrl.c
>>>> +++ b/drivers/clk/ti/clkctrl.c
>>>> @@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
>>>>           return false;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
>>>> +
>>>> +/**
>>>> + * ti_clk_notify_resets - Notify the clock driver associated reset status
>>>
>>> This is completely unused in this patch series. What's going on?
>>
>> This is needed by the OMAP reset driver. See:
>>
>> https://lwn.net/Articles/797597/
>>
> 
> Ok. I decided to punt this topic forward to next release at the least.
> To clarify, TI is not special with regards to coordinating resets and
> clk enable/disable state. Every other silicon vendor has the same
> requirements and nobody is doing a good job at it.
> 
> Please devise a way that avoids making a tight coupling between the clk
> driver and the reset driver in this way. Are the two in the same
> register space?

No, they do not share register space. One is under a PRM node, one is 
under CM node, and there are multiple instances of each following each 
other:

prm-1
prm-2
prm-3

...

cm-1
cm-2
cm-3

And the gap between PRM + CM nodes is multiple megabytes in register 
space. To make things worse, there are some mutant CM nodes in the 
middle of the PRM nodes on certain SoCs.

  Perhaps we need to combine the two drivers then. Or can
> this be implemented as a genpd that coordinates the resets and clk
> controls for various devices?

Generally, ti-sysc bus driver is the one doing the trick combining reset 
+ clock handling. However, this is linked at the pm-runtime on device 
level so it imposes certain sequencing due to way kernel PM is 
implemented. Basically we can't enable the clocks + deassert reset for 
remoteproc before the driver is able to load up the firmware for it. 
Maybe if I add a custom genpd or just custom PM runtime for the 
remoteprocs that would handle both clk + reset...

Another potential change I can think of here is that I would add resets 
property under the clkctrl nodes, and link them via DT handles. The 
clock driver would get a handle to the reset controller, and query its 
state via generic API instead of adding this custom one. I would still 
need to add a separate custom API for telling the clocks that reset 
controller is in place though... And this would still be a hard link 
between reset + clocks.

Do you think fully custom PM implementation would be better here which 
would just control reset + clock signals directly?

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Stephen Boyd]

Quoting Tero Kristo (2019-09-06 12:57:06)
> On 06/09/2019 19:15, Stephen Boyd wrote:
> > Quoting Tero Kristo (2019-08-29 23:06:41)
> >> On 29/08/2019 23:05, Stephen Boyd wrote:
> >>> Quoting Tero Kristo (2019-08-27 23:59:27)
> >>>> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
> >>>> index e3e0a66a6ce2..47a0d1398c6f 100644
> >>>> --- a/drivers/clk/ti/clkctrl.c
> >>>> +++ b/drivers/clk/ti/clkctrl.c
> >>>> @@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
> >>>>           return false;
> >>>>    }
> >>>>    EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
> >>>> +
> >>>> +/**
> >>>> + * ti_clk_notify_resets - Notify the clock driver associated reset status
> >>>
> >>> This is completely unused in this patch series. What's going on?
> >>
> >> This is needed by the OMAP reset driver. See:
> >>
> >> https://lwn.net/Articles/797597/
> >>
> > 
> > Ok. I decided to punt this topic forward to next release at the least.
> > To clarify, TI is not special with regards to coordinating resets and
> > clk enable/disable state. Every other silicon vendor has the same
> > requirements and nobody is doing a good job at it.
> > 
> > Please devise a way that avoids making a tight coupling between the clk
> > driver and the reset driver in this way. Are the two in the same
> > register space?
> 
> No, they do not share register space. One is under a PRM node, one is 
> under CM node, and there are multiple instances of each following each 
> other:
> 
> prm-1
> prm-2
> prm-3

So PRM is reset?

> 
> ...
> 
> cm-1
> cm-2
> cm-3

And CM is clk?

> 
> And the gap between PRM + CM nodes is multiple megabytes in register 
> space. To make things worse, there are some mutant CM nodes in the 
> middle of the PRM nodes on certain SoCs.

Ok, sounds fair!

> 
>   Perhaps we need to combine the two drivers then. Or can
> > this be implemented as a genpd that coordinates the resets and clk
> > controls for various devices?
> 
> Generally, ti-sysc bus driver is the one doing the trick combining reset 
> + clock handling. However, this is linked at the pm-runtime on device 
> level so it imposes certain sequencing due to way kernel PM is 
> implemented. Basically we can't enable the clocks + deassert reset for 
> remoteproc before the driver is able to load up the firmware for it. 
> Maybe if I add a custom genpd or just custom PM runtime for the 
> remoteprocs that would handle both clk + reset...
> 
> Another potential change I can think of here is that I would add resets 
> property under the clkctrl nodes, and link them via DT handles. The 
> clock driver would get a handle to the reset controller, and query its 
> state via generic API instead of adding this custom one. I would still 
> need to add a separate custom API for telling the clocks that reset 
> controller is in place though... And this would still be a hard link 
> between reset + clocks.
> 
> Do you think fully custom PM implementation would be better here which 
> would just control reset + clock signals directly?
> 

From what you're saying it sounds like a job for genpds. Maybe genpds
aren't up to the task yet, but we want devices that have resets and clks
going to them to manage the order of operations somehow without having
to "lie" and say that the resets go to the clk controller when they
don't (or vice versa).

Re: [PATCHv2 4/6] clk: ti: clkctrl: add API to notify reset status

[Tero Kristo]

On 06/09/2019 23:37, Stephen Boyd wrote:
> Quoting Tero Kristo (2019-09-06 12:57:06)
>> On 06/09/2019 19:15, Stephen Boyd wrote:
>>> Quoting Tero Kristo (2019-08-29 23:06:41)
>>>> On 29/08/2019 23:05, Stephen Boyd wrote:
>>>>> Quoting Tero Kristo (2019-08-27 23:59:27)
>>>>>> diff --git a/drivers/clk/ti/clkctrl.c b/drivers/clk/ti/clkctrl.c
>>>>>> index e3e0a66a6ce2..47a0d1398c6f 100644
>>>>>> --- a/drivers/clk/ti/clkctrl.c
>>>>>> +++ b/drivers/clk/ti/clkctrl.c
>>>>>> @@ -680,3 +689,38 @@ u32 ti_clk_is_in_standby(struct clk *clk)
>>>>>>            return false;
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(ti_clk_is_in_standby);
>>>>>> +
>>>>>> +/**
>>>>>> + * ti_clk_notify_resets - Notify the clock driver associated reset status
>>>>>
>>>>> This is completely unused in this patch series. What's going on?
>>>>
>>>> This is needed by the OMAP reset driver. See:
>>>>
>>>> https://lwn.net/Articles/797597/
>>>>
>>>
>>> Ok. I decided to punt this topic forward to next release at the least.
>>> To clarify, TI is not special with regards to coordinating resets and
>>> clk enable/disable state. Every other silicon vendor has the same
>>> requirements and nobody is doing a good job at it.
>>>
>>> Please devise a way that avoids making a tight coupling between the clk
>>> driver and the reset driver in this way. Are the two in the same
>>> register space?
>>
>> No, they do not share register space. One is under a PRM node, one is
>> under CM node, and there are multiple instances of each following each
>> other:
>>
>> prm-1
>> prm-2
>> prm-3
> 
> So PRM is reset?

PRM is for Power and Reset Manager.

> 
>>
>> ...
>>
>> cm-1
>> cm-2
>> cm-3
> 
> And CM is clk?

CM is for Clock Manager.

So basically for both answer is yes.

> 
>>
>> And the gap between PRM + CM nodes is multiple megabytes in register
>> space. To make things worse, there are some mutant CM nodes in the
>> middle of the PRM nodes on certain SoCs.
> 
> Ok, sounds fair!
> 
>>
>>    Perhaps we need to combine the two drivers then. Or can
>>> this be implemented as a genpd that coordinates the resets and clk
>>> controls for various devices?
>>
>> Generally, ti-sysc bus driver is the one doing the trick combining reset
>> + clock handling. However, this is linked at the pm-runtime on device
>> level so it imposes certain sequencing due to way kernel PM is
>> implemented. Basically we can't enable the clocks + deassert reset for
>> remoteproc before the driver is able to load up the firmware for it.
>> Maybe if I add a custom genpd or just custom PM runtime for the
>> remoteprocs that would handle both clk + reset...
>>
>> Another potential change I can think of here is that I would add resets
>> property under the clkctrl nodes, and link them via DT handles. The
>> clock driver would get a handle to the reset controller, and query its
>> state via generic API instead of adding this custom one. I would still
>> need to add a separate custom API for telling the clocks that reset
>> controller is in place though... And this would still be a hard link
>> between reset + clocks.
>>
>> Do you think fully custom PM implementation would be better here which
>> would just control reset + clock signals directly?
>>
> 
>  From what you're saying it sounds like a job for genpds. Maybe genpds
> aren't up to the task yet, but we want devices that have resets and clks
> going to them to manage the order of operations somehow without having
> to "lie" and say that the resets go to the clk controller when they
> don't (or vice versa).

Yeah I am not too sure if genpd would suit this purpose as it has no 
support for reset control so far I believe. However I think the custom 
PM implementation might. I will give it a shot next week and see how it 
fares. Basically the main issue I am trying to tackle is not to 
introduce any timeouts anywhere due to the hardware level dependencies 
of these two guys.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki