* [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent
@ 2017-07-03 14:28 Adriana Reus
2017-07-03 14:28 ` [RFC 1/2] clk: imx: Add CLK_SET_RATE_GATE for imx7d clocks Adriana Reus
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Adriana Reus @ 2017-07-03 14:28 UTC (permalink / raw)
To: linux-arm-kernel
Hi all,
I am currently trying to fix an issue occurring when trying to set the
CLK_SET_RATE_GATE flag for the uart root clks. This causes the imx serial
driver to fail when setting up devicetree specified frequency or parent because
the uart clk is already in use by the ccm driver that starts up all uart clocks
for earlycon.
Some context:
There are three main types of clock slices on imx7d ccm: "Core clock slice",
"Bus clock slice (AXI/AHB)", and "Peripheral clock slice". Vast majority of
clock roots fall in the peripheral slice category:
8:1 mux -> gate -> div -> div -> gate
The root clocks that derive from peripheral slices are expected to be
gated when changing frequency [0].
The first patch of this series sets this up. (It is only a reference point for
this conversation, not to be merged as it WIP and needs more testing).
When setting this up, the imx serial driver
(platform_driver_register -> of_clk_set_defaults) will fail
to set the rate or parent specified in the devicetree:
imx-uart 30a70000.serial: clk_set_rate() failed
imx-uart: probe of 30a70000.serial failed with error -16
The reason for this is that the uart clks are already prepared and enabled in
imx7d_clocks_init by the imx_register_uart_clocks function if earlycon is
enabled. Functionality added in patches:
'commit 55adc61c568af99419be1dc0412f8eae019c71f2
("clk: imx: add common logic to detect early UART usage")'
'commit 1b9af68f325cb91ac9fc691f52d69dfb0826afd7
("clk: imx7d: retain early UART clocks during kernel init")'
One option is to remove the imx_register_uart_clocks from the ccm driver and
leave the uart clock set up and enable entirely to the serial driver and DT.
The second RFC patch does this for imx7d.
That would mean that we would rely on the bootloader to enable these clocks for
earlycon. This is what other platforms seem to do, so it seems like
a reasonable asumption that the bootloader would set it up (on my tested
u-boot + imx7d setup, it works).
The topic was also briefly addressed in this thread [1] and as it is stated by
Lucas Stach in the thread, earlycon must work before the clock driver is
initialized anyway.
If it is agreed that it's best to remove it I can add a patch to remove it from
all imx platforms, otherwise I am open to any other alternatives.
Any feedback is apreciated and thanks for reading thus far.
[0]: http://www.nxp.com/docs/en/reference-manual/IMX7DRM.pdf
Relevant sections: 5.2.4; 5.1.3.5.3; 5.2.6.4 [5.2.6.4.3]
[1]: https://patchwork.kernel.org/patch/8979421/
Adriana Reus (2):
clk: imx: Add CLK_SET_RATE_GATE for imx7d clocks
clk: imx: Remove enabling uart clocks from ccm driver.
drivers/clk/imx/clk-imx7d.c | 32 +++++++++++---------------------
drivers/clk/imx/clk.h | 18 +++++++++++++-----
2 files changed, 24 insertions(+), 26 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC 1/2] clk: imx: Add CLK_SET_RATE_GATE for imx7d clocks
2017-07-03 14:28 [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Adriana Reus
@ 2017-07-03 14:28 ` Adriana Reus
2017-07-03 14:28 ` [RFC 2/2] clk: imx: Remove enabling uart clocks from ccm driver Adriana Reus
2017-07-03 14:42 ` [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Lucas Stach
2 siblings, 0 replies; 7+ messages in thread
From: Adriana Reus @ 2017-07-03 14:28 UTC (permalink / raw)
To: linux-arm-kernel
There are three main types of clock slices on imx7d ccm:
"Core clock slice", "Bus clock slice (AXI/AHB)", and "Peripheral clock
slice".
Documentation and general overview: [0]
Vast majority of clock roots fall in the peripheral slice category:
8:1 mux -> gate -> div -> div -> gate
The root clocks that derive from peripheral slices are expected to be
gated when changing frequency.[1]
Introduce imx_clk_gate2_flags for clocks where it is not necessary to
set RATE_GATE and add the CLK_SET_RATE flag to imx_clk_gate4 for the
rest of the clocks.
[0]: http://www.nxp.com/docs/en/reference-manual/IMX7DRM.pdf
Relevant sections: 5.2.4
[1]: Relevant sections: 5.1.3.5.3; 5.2.6.4 [5.2.6.4.3]
Signed-off-by: Anson Huang <b20788@freescale.com>
Signed-off-by: Adriana Reus <adriana.reus@nxp.com>
---
drivers/clk/imx/clk-imx7d.c | 16 +++++++++++-----
drivers/clk/imx/clk.h | 18 +++++++++++++-----
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 3da1218..e1c31f5 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -791,11 +791,17 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div", "clko1_pre_div", base + 0xbd80, 0, 6);
clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div", "clko2_pre_div", base + 0xbe00, 0, 6);
- clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk", "arm_a7_div", base + 0x4000, 0);
- clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk", "arm_m4_div", base + 0x4010, 0);
- clks[IMX7D_ARM_M0_ROOT_CLK] = imx_clk_gate4("arm_m0_root_clk", "arm_m0_div", base + 0x4020, 0);
- clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate4("main_axi_root_clk", "axi_post_div", base + 0x4040, 0);
- clks[IMX7D_DISP_AXI_ROOT_CLK] = imx_clk_gate4("disp_axi_root_clk", "disp_axi_post_div", base + 0x4050, 0);
+ clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate2_flags("arm_a7_root_clk", "arm_a7_div", base + 0x4000, 0,
+ CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
+ clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate2_flags("arm_m4_root_clk", "arm_m4_div", base + 0x4010, 0,
+ CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
+ clks[IMX7D_ARM_M0_ROOT_CLK] = imx_clk_gate2_flags("arm_m0_root_clk", "arm_m0_div", base + 0x4020, 0,
+ CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
+ clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate2_flags("main_axi_root_clk", "axi_post_div", base + 0x4040, 0,
+ CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
+ clks[IMX7D_DISP_AXI_ROOT_CLK] = imx_clk_gate2_flags("disp_axi_root_clk", "disp_axi_post_div",
+ base + 0x4050, 0,
+ CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
clks[IMX7D_ENET_AXI_ROOT_CLK] = imx_clk_gate4("enet_axi_root_clk", "enet_axi_post_div", base + 0x4060, 0);
clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk", "axi_post_div", base + 0x4110, 0);
clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk", "ahb_root_clk", base + 0x4120, 0);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index d54f072..d0bc094 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -149,8 +149,9 @@ static inline struct clk *imx_clk_gate2_shared2(const char *name,
unsigned int *share_count)
{
return clk_register_gate2(NULL, name, parent, CLK_SET_RATE_PARENT |
- CLK_OPS_PARENT_ENABLE, reg, shift, 0x3, 0,
- &imx_ccm_lock, share_count);
+ CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_GATE,
+ reg, shift, 0x3, 0, &imx_ccm_lock,
+ share_count);
}
static inline struct clk *imx_clk_gate2_cgr(const char *name,
@@ -171,9 +172,16 @@ static inline struct clk *imx_clk_gate3(const char *name, const char *parent,
static inline struct clk *imx_clk_gate4(const char *name, const char *parent,
void __iomem *reg, u8 shift)
{
- return clk_register_gate2(NULL, name, parent,
- CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE,
- reg, shift, 0x3, 0, &imx_ccm_lock, NULL);
+ return clk_register_gate2(NULL, name, parent, CLK_SET_RATE_PARENT |
+ CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_GATE,
+ reg, shift, 0x3, 0, &imx_ccm_lock, NULL);
+}
+
+static inline struct clk *imx_clk_gate2_flags(const char *name, const char *parent,
+ void __iomem *reg, u8 shift, unsigned long flags)
+{
+ return clk_register_gate2(NULL, name, parent, flags, reg, shift, 0x3,
+ 0, &imx_ccm_lock, NULL);
}
static inline struct clk *imx_clk_mux(const char *name, void __iomem *reg,
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC 2/2] clk: imx: Remove enabling uart clocks from ccm driver.
2017-07-03 14:28 [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Adriana Reus
2017-07-03 14:28 ` [RFC 1/2] clk: imx: Add CLK_SET_RATE_GATE for imx7d clocks Adriana Reus
@ 2017-07-03 14:28 ` Adriana Reus
2017-07-03 14:42 ` [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Lucas Stach
2 siblings, 0 replies; 7+ messages in thread
From: Adriana Reus @ 2017-07-03 14:28 UTC (permalink / raw)
To: linux-arm-kernel
Remove uart clock enable functionality from ccm driver and
leave uart clocks enabling and setup to the serial driver.
This functionality was introduced by patch
"b9af68f clk: imx7d: retain early UART clocks during kernel init"
to ensure that for earlycon and earlyprink we have the uart port enabled
even if the bootloader does not enable it.
However, on imx7d uart root clocks are recommended to be gated when
changing frequency and if the CLK_SET_RATE_GATE is set for these clocks
this means that the serial driver will fail to re-parent or set_frequency
via device tree attributes on them because the ccm driver has them all in use.
Signed-off-by: Adriana Reus <adriana.reus@nxp.com>
---
drivers/clk/imx/clk-imx7d.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index e1c31f5..374ec0f 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -392,17 +392,6 @@ static int const clks_init_on[] __initconst = {
static struct clk_onecell_data clk_data;
-static struct clk ** const uart_clks[] __initconst = {
- &clks[IMX7D_UART1_ROOT_CLK],
- &clks[IMX7D_UART2_ROOT_CLK],
- &clks[IMX7D_UART3_ROOT_CLK],
- &clks[IMX7D_UART4_ROOT_CLK],
- &clks[IMX7D_UART5_ROOT_CLK],
- &clks[IMX7D_UART6_ROOT_CLK],
- &clks[IMX7D_UART7_ROOT_CLK],
- NULL
-};
-
static void __init imx7d_clocks_init(struct device_node *ccm_node)
{
struct device_node *np;
@@ -897,10 +886,5 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
/* use old gpt clk setting, gpt1 root clk must be twice as gpt counter freq */
clk_set_parent(clks[IMX7D_GPT1_ROOT_SRC], clks[IMX7D_OSC_24M_CLK]);
- /* set uart module clock's parent clock source that must be great then 80MHz */
- clk_set_parent(clks[IMX7D_UART1_ROOT_SRC], clks[IMX7D_OSC_24M_CLK]);
-
- imx_register_uart_clocks(uart_clks);
-
}
CLK_OF_DECLARE(imx7d, "fsl,imx7d-ccm", imx7d_clocks_init);
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent
2017-07-03 14:28 [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Adriana Reus
2017-07-03 14:28 ` [RFC 1/2] clk: imx: Add CLK_SET_RATE_GATE for imx7d clocks Adriana Reus
2017-07-03 14:28 ` [RFC 2/2] clk: imx: Remove enabling uart clocks from ccm driver Adriana Reus
@ 2017-07-03 14:42 ` Lucas Stach
2017-07-04 10:57 ` Adriana Reus
2 siblings, 1 reply; 7+ messages in thread
From: Lucas Stach @ 2017-07-03 14:42 UTC (permalink / raw)
To: linux-arm-kernel
Am Montag, den 03.07.2017, 17:28 +0300 schrieb Adriana Reus:
> Hi all,
>
> I am currently trying to fix an issue occurring when trying to set the
> CLK_SET_RATE_GATE flag for the uart root clks. This causes the imx serial
> driver to fail when setting up devicetree specified frequency or parent because
> the uart clk is already in use by the ccm driver that starts up all uart clocks
> for earlycon.
>
> Some context:
>
> There are three main types of clock slices on imx7d ccm: "Core clock slice",
> "Bus clock slice (AXI/AHB)", and "Peripheral clock slice". Vast majority of
> clock roots fall in the peripheral slice category:
>
> 8:1 mux -> gate -> div -> div -> gate
>
> The root clocks that derive from peripheral slices are expected to be
> gated when changing frequency [0].
>
> The first patch of this series sets this up. (It is only a reference point for
> this conversation, not to be merged as it WIP and needs more testing).
>
> When setting this up, the imx serial driver
> (platform_driver_register -> of_clk_set_defaults) will fail
> to set the rate or parent specified in the devicetree:
>
> imx-uart 30a70000.serial: clk_set_rate() failed
> imx-uart: probe of 30a70000.serial failed with error -16
>
> The reason for this is that the uart clks are already prepared and enabled in
> imx7d_clocks_init by the imx_register_uart_clocks function if earlycon is
> enabled. Functionality added in patches:
> 'commit 55adc61c568af99419be1dc0412f8eae019c71f2
> ("clk: imx: add common logic to detect early UART usage")'
> 'commit 1b9af68f325cb91ac9fc691f52d69dfb0826afd7
> ("clk: imx7d: retain early UART clocks during kernel init")'
>
> One option is to remove the imx_register_uart_clocks from the ccm driver and
> leave the uart clock set up and enable entirely to the serial driver and DT.
> The second RFC patch does this for imx7d.
This is not an option. The CCM holds a reference to those clocks,
specifically to prevent them from being disabled before a real serial
driver is bound. If this isn't done you might end up in a situation
where PM or driver probe deferal disables the UART clock/the parent of
this clock, even if it was previously enabled by the bootloader. With
earlycon this might lead to a full system hang at this point, when
earlycon tries to access a clock gated UART.
Regards,
Lucas
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent
2017-07-03 14:42 ` [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Lucas Stach
@ 2017-07-04 10:57 ` Adriana Reus
2017-07-04 11:01 ` Lucas Stach
0 siblings, 1 reply; 7+ messages in thread
From: Adriana Reus @ 2017-07-04 10:57 UTC (permalink / raw)
To: linux-arm-kernel
On Lu, 2017-07-03 at 16:42 +0200, Lucas Stach wrote:
> Am Montag, den 03.07.2017, 17:28 +0300 schrieb Adriana Reus:
> >
> > Hi all,
> >
> > I am currently trying to fix an issue occurring when trying to set
> > the
> > CLK_SET_RATE_GATE flag for the uart root clks. This causes the imx
> > serial
> > driver to fail when setting up devicetree specified frequency or
> > parent because
> > the uart clk is already in use by the ccm driver that starts up all
> > uart clocks
> > for earlycon.
> >
> > Some context:
> >
> > There are three main types of clock slices on imx7d ccm: "Core
> > clock slice",
> > "Bus clock slice (AXI/AHB)", and "Peripheral clock slice". Vast
> > majority of
> > clock roots fall in the peripheral slice category:
> >
> > ????8:1 mux -> gate -> div -> div -> gate
> >
> > The root clocks that derive from peripheral slices are expected to
> > be
> > gated when changing frequency [0].
> >
> > The first patch of this series sets this up. (It is only a
> > reference point for
> > this conversation, not to be merged as it WIP and needs more
> > testing).
> >
> > When setting this up, the imx serial driver
> > (platform_driver_register -> of_clk_set_defaults) will fail
> > to set the rate or parent specified in the devicetree:
> >
> > ????imx-uart 30a70000.serial: clk_set_rate() failed
> > ????imx-uart: probe of 30a70000.serial failed with error -16
> >
> > The reason for this is that the uart clks are already prepared and
> > enabled in
> > imx7d_clocks_init by the imx_register_uart_clocks function if
> > earlycon is
> > enabled. Functionality added in patches:
> > 'commit 55adc61c568af99419be1dc0412f8eae019c71f2
> > ("clk: imx: add common logic to detect early UART usage")'
> > 'commit 1b9af68f325cb91ac9fc691f52d69dfb0826afd7
> > ("clk: imx7d: retain early UART clocks during kernel init")'
> >
> > One option is to remove the imx_register_uart_clocks from the ccm
> > driver and
> > leave the uart clock set up and enable entirely to the serial
> > driver and DT.
> > The second RFC patch does this for imx7d.
>
> This is not an option. The CCM holds a reference to those clocks,
> specifically to prevent them from being disabled before a real serial
> driver is bound. If this isn't done you might end up in a situation
> where PM or driver probe deferal disables the UART clock/the parent
> of
> this clock, even if it was previously enabled by the bootloader. With
> earlycon this might lead to a full system hang at this point, when
> earlycon tries to access a clock gated UART.
>
> Regards,
> Lucas
>
Hi Lucas, Thank you for your reply.
Looking at clk_core_unprepare and clk_core_disable they seem to return
without hardware disabling the clock if enable_count or prepare_count
are 0. So, disabling a bootloader enabled only clock does not seem to
be a problem. Let me know if I misunderstand.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent
2017-07-04 10:57 ` Adriana Reus
@ 2017-07-04 11:01 ` Lucas Stach
2017-07-05 9:26 ` Adriana Reus
0 siblings, 1 reply; 7+ messages in thread
From: Lucas Stach @ 2017-07-04 11:01 UTC (permalink / raw)
To: linux-arm-kernel
Am Dienstag, den 04.07.2017, 10:57 +0000 schrieb Adriana Reus:
> On Lu, 2017-07-03 at 16:42 +0200, Lucas Stach wrote:
> > Am Montag, den 03.07.2017, 17:28 +0300 schrieb Adriana Reus:
> > >
> > > Hi all,
> > >
> > > I am currently trying to fix an issue occurring when trying to set
> > > the
> > > CLK_SET_RATE_GATE flag for the uart root clks. This causes the imx
> > > serial
> > > driver to fail when setting up devicetree specified frequency or
> > > parent because
> > > the uart clk is already in use by the ccm driver that starts up all
> > > uart clocks
> > > for earlycon.
> > >
> > > Some context:
> > >
> > > There are three main types of clock slices on imx7d ccm: "Core
> > > clock slice",
> > > "Bus clock slice (AXI/AHB)", and "Peripheral clock slice". Vast
> > > majority of
> > > clock roots fall in the peripheral slice category:
> > >
> > > 8:1 mux -> gate -> div -> div -> gate
> > >
> > > The root clocks that derive from peripheral slices are expected to
> > > be
> > > gated when changing frequency [0].
> > >
> > > The first patch of this series sets this up. (It is only a
> > > reference point for
> > > this conversation, not to be merged as it WIP and needs more
> > > testing).
> > >
> > > When setting this up, the imx serial driver
> > > (platform_driver_register -> of_clk_set_defaults) will fail
> > > to set the rate or parent specified in the devicetree:
> > >
> > > imx-uart 30a70000.serial: clk_set_rate() failed
> > > imx-uart: probe of 30a70000.serial failed with error -16
> > >
> > > The reason for this is that the uart clks are already prepared and
> > > enabled in
> > > imx7d_clocks_init by the imx_register_uart_clocks function if
> > > earlycon is
> > > enabled. Functionality added in patches:
> > > 'commit 55adc61c568af99419be1dc0412f8eae019c71f2
> > > ("clk: imx: add common logic to detect early UART usage")'
> > > 'commit 1b9af68f325cb91ac9fc691f52d69dfb0826afd7
> > > ("clk: imx7d: retain early UART clocks during kernel init")'
> > >
> > > One option is to remove the imx_register_uart_clocks from the ccm
> > > driver and
> > > leave the uart clock set up and enable entirely to the serial
> > > driver and DT.
> > > The second RFC patch does this for imx7d.
> >
> > This is not an option. The CCM holds a reference to those clocks,
> > specifically to prevent them from being disabled before a real serial
> > driver is bound. If this isn't done you might end up in a situation
> > where PM or driver probe deferal disables the UART clock/the parent
> > of
> > this clock, even if it was previously enabled by the bootloader. With
> > earlycon this might lead to a full system hang at this point, when
> > earlycon tries to access a clock gated UART.
> >
> > Regards,
> > Lucas
> >
>
> Hi Lucas, Thank you for your reply.
>
> Looking at clk_core_unprepare and clk_core_disable they seem to return
> without hardware disabling the clock if enable_count or prepare_count
> are 0. So, disabling a bootloader enabled only clock does not seem to
> be a problem. Let me know if I misunderstand.
That's correct, however image a situation where some other driver probes
that uses the same parent clock as the earlycon UART, before the real
serial driver is up. This driver requests and enables the parent clock,
then goes into runtime suspended state, disabling the clock. As the
clock has been enabled by this driver we now have the transition from
enabled to disabled, disabling the bootloader enabled clock.
The same can happen with driver probe deferal.
Regards,
Lucas
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent
2017-07-04 11:01 ` Lucas Stach
@ 2017-07-05 9:26 ` Adriana Reus
0 siblings, 0 replies; 7+ messages in thread
From: Adriana Reus @ 2017-07-05 9:26 UTC (permalink / raw)
To: linux-arm-kernel
On Ma, 2017-07-04 at 13:01 +0200, Lucas Stach wrote:
> Am Dienstag, den 04.07.2017, 10:57 +0000 schrieb Adriana Reus:
> >
> > On Lu, 2017-07-03 at 16:42 +0200, Lucas Stach wrote:
> > >
> > > Am Montag, den 03.07.2017, 17:28 +0300 schrieb Adriana Reus:
> > > >
> > > >
> > > > Hi all,
> > > >
> > > > I am currently trying to fix an issue occurring when trying to
> > > > set
> > > > the
> > > > CLK_SET_RATE_GATE flag for the uart root clks. This causes the
> > > > imx
> > > > serial
> > > > driver to fail when setting up devicetree specified frequency
> > > > or
> > > > parent because
> > > > the uart clk is already in use by the ccm driver that starts up
> > > > all
> > > > uart clocks
> > > > for earlycon.
> > > >
> > > > Some context:
> > > >
> > > > There are three main types of clock slices on imx7d ccm: "Core
> > > > clock slice",
> > > > "Bus clock slice (AXI/AHB)", and "Peripheral clock slice". Vast
> > > > majority of
> > > > clock roots fall in the peripheral slice category:
> > > >
> > > > ????8:1 mux -> gate -> div -> div -> gate
> > > >
> > > > The root clocks that derive from peripheral slices are expected
> > > > to
> > > > be
> > > > gated when changing frequency [0].
> > > >
> > > > The first patch of this series sets this up. (It is only a
> > > > reference point for
> > > > this conversation, not to be merged as it WIP and needs more
> > > > testing).
> > > >
> > > > When setting this up, the imx serial driver
> > > > (platform_driver_register -> of_clk_set_defaults) will fail
> > > > to set the rate or parent specified in the devicetree:
> > > >
> > > > ????imx-uart 30a70000.serial: clk_set_rate() failed
> > > > ????imx-uart: probe of 30a70000.serial failed with error -16
> > > >
> > > > The reason for this is that the uart clks are already prepared
> > > > and
> > > > enabled in
> > > > imx7d_clocks_init by the imx_register_uart_clocks function if
> > > > earlycon is
> > > > enabled. Functionality added in patches:
> > > > 'commit 55adc61c568af99419be1dc0412f8eae019c71f2
> > > > ("clk: imx: add common logic to detect early UART usage")'
> > > > 'commit 1b9af68f325cb91ac9fc691f52d69dfb0826afd7
> > > > ("clk: imx7d: retain early UART clocks during kernel init")'
> > > >
> > > > One option is to remove the imx_register_uart_clocks from the
> > > > ccm
> > > > driver and
> > > > leave the uart clock set up and enable entirely to the serial
> > > > driver and DT.
> > > > The second RFC patch does this for imx7d.
> > >
> > > This is not an option. The CCM holds a reference to those clocks,
> > > specifically to prevent them from being disabled before a real
> > > serial
> > > driver is bound. If this isn't done you might end up in a
> > > situation
> > > where PM or driver probe deferal disables the UART clock/the
> > > parent
> > > of
> > > this clock, even if it was previously enabled by the bootloader.
> > > With
> > > earlycon this might lead to a full system hang at this point,
> > > when
> > > earlycon tries to access a clock gated UART.
> > >
> > > Regards,
> > > Lucas
> > >
> >
> > Hi Lucas, Thank you for your reply.
> >
> > Looking at clk_core_unprepare and clk_core_disable they seem to
> > return
> > without hardware disabling the clock if enable_count or
> > prepare_count
> > are 0. So, disabling a bootloader enabled only clock does not seem
> > to
> > be a problem. Let me know if I misunderstand.
>
> That's correct, however image a situation where some other driver
> probes
> that uses the same parent clock as the earlycon UART, before the real
> serial driver is up. This driver requests and enables the parent
> clock,
> then goes into runtime suspended state, disabling the clock. As the
> clock has been enabled by this driver we now have the transition from
> enabled to disabled, disabling the bootloader enabled clock.
>
> The same can happen with driver probe deferal.
>
> Regards,
> Lucas
>
Ok, thanks for further explaining.
Sounds like this would be a general theoretical case, not
bound to a specific platform, and possibly could apply to other
booloader-on clocks, I guess.
I am not sure as to what a proper solution should be.
Would adding logic in Clock Framework to handle
bootloader-on clocks differently be an option ?
Thanks,
Adriana
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-05 9:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03 14:28 [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Adriana Reus
2017-07-03 14:28 ` [RFC 1/2] clk: imx: Add CLK_SET_RATE_GATE for imx7d clocks Adriana Reus
2017-07-03 14:28 ` [RFC 2/2] clk: imx: Remove enabling uart clocks from ccm driver Adriana Reus
2017-07-03 14:42 ` [RFC 0/2] clk: imx7d: Uart clock set-up conflict if clk needs gating on set rate or re-parent Lucas Stach
2017-07-04 10:57 ` Adriana Reus
2017-07-04 11:01 ` Lucas Stach
2017-07-05 9:26 ` Adriana Reus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).