* [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks @ 2016-07-07 12:13 Andi Shyti 2016-07-07 12:13 ` [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks Andi Shyti ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Andi Shyti @ 2016-07-07 12:13 UTC (permalink / raw) To: Chanwoo Choi Cc: Jaehoon Chung, Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti, Andi Shyti Hi, I managed to bring some confusion on these two patches, but finally I managed to find the right solution (which is similar to the patch version 2). The real issue was hidden in the spi bus as reported in the following patch: https://lkml.org/lkml/2016/7/7/81 This patch in principle replaces the clock_ignore_unused flags in for the spi and enables the spi3 that in the tm2(e) board will be connected to the IR remote controller. Changelog: V1 -> V2 - the "sclk_spi3" doesn't need to be enabled in boot time as it is handled by the spi driver itself. - use the CLK_IS_CRITICAL flag for the ioclk V2 -> V3 - some more tests has confirmed taht "sclk_spi1,3" need to be enabled as critical! - added Chanwoo's review in the second commit. V3 -> V4 - patch V3 was wrong, back to patch v2. - reworded the commit of patch no. 1 Thanks, Andi Andi Shyti (2): clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks clk: exynos5433: enable sclk_ioclk for SPI3 drivers/clk/samsung/clk-exynos5433.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) -- 2.8.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks 2016-07-07 12:13 [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Andi Shyti @ 2016-07-07 12:13 ` Andi Shyti 2016-07-07 12:46 ` Krzysztof Kozlowski 2016-07-07 12:13 ` [PATCH v4 2/2] clk: exynos5433: enable sclk_ioclk for SPI3 Andi Shyti 2016-07-07 13:18 ` [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Sylwester Nawrocki 2 siblings, 1 reply; 9+ messages in thread From: Andi Shyti @ 2016-07-07 12:13 UTC (permalink / raw) To: Chanwoo Choi Cc: Jaehoon Chung, Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti, Andi Shyti The CLK_IGNORE_UNUSED flag has to be avoided whenever possible. Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks, which enables the clock line during boot time. Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> Signed-off-by: Andi Shyti <andi.shyti@samsung.com> --- drivers/clk/samsung/clk-exynos5433.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c index c3a5318..337387b 100644 --- a/drivers/clk/samsung/clk-exynos5433.c +++ b/drivers/clk/samsung/clk-exynos5433.c @@ -1662,7 +1662,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = { ENABLE_SCLK_PERIC, 13, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_IOCLK_SPI1, "sclk_ioclk_spi1", "ioclk_spi1_clk_in", ENABLE_SCLK_PERIC, 12, - CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0), + CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_IOCLK_SPI0, "sclk_ioclk_spi0", "ioclk_spi0_clk_in", ENABLE_SCLK_PERIC, 11, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_IOCLK_I2S1_BCLK, "sclk_ioclk_i2s1_bclk", @@ -1677,7 +1677,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = { GATE(CLK_SCLK_SPI2, "sclk_spi2", "sclk_spi2_peric", ENABLE_SCLK_PERIC, 5, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_SPI1, "sclk_spi1", "sclk_spi1_peric", ENABLE_SCLK_PERIC, - 4, CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0), + 4, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_SPI0, "sclk_spi0", "sclk_spi0_peric", ENABLE_SCLK_PERIC, 3, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_UART2, "sclk_uart2", "sclk_uart2_peric", -- 2.8.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks 2016-07-07 12:13 ` [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks Andi Shyti @ 2016-07-07 12:46 ` Krzysztof Kozlowski 2016-07-07 13:27 ` Andi Shyti 0 siblings, 1 reply; 9+ messages in thread From: Krzysztof Kozlowski @ 2016-07-07 12:46 UTC (permalink / raw) To: Andi Shyti, Chanwoo Choi Cc: Jaehoon Chung, Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti On 07/07/2016 02:13 PM, Andi Shyti wrote: > The CLK_IGNORE_UNUSED flag has to be avoided whenever possible. > Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks, > which enables the clock line during boot time. I don't agree. Both flags should be avoided. Clk is critical does not solve the problem. It is just a better workaround for lack of proper clock consumers. The IOCLK is not a critical clock. It can be disabled (e.g. when SoC is used on a board without any SPI device connected). Best regards, Krzysztof > > Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> > Signed-off-by: Andi Shyti <andi.shyti@samsung.com> > --- > drivers/clk/samsung/clk-exynos5433.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c > index c3a5318..337387b 100644 > --- a/drivers/clk/samsung/clk-exynos5433.c > +++ b/drivers/clk/samsung/clk-exynos5433.c > @@ -1662,7 +1662,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = { > ENABLE_SCLK_PERIC, 13, CLK_SET_RATE_PARENT, 0), > GATE(CLK_SCLK_IOCLK_SPI1, "sclk_ioclk_spi1", "ioclk_spi1_clk_in", > ENABLE_SCLK_PERIC, 12, > - CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0), > + CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0), > GATE(CLK_SCLK_IOCLK_SPI0, "sclk_ioclk_spi0", "ioclk_spi0_clk_in", > ENABLE_SCLK_PERIC, 11, CLK_SET_RATE_PARENT, 0), > GATE(CLK_SCLK_IOCLK_I2S1_BCLK, "sclk_ioclk_i2s1_bclk", > @@ -1677,7 +1677,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = { > GATE(CLK_SCLK_SPI2, "sclk_spi2", "sclk_spi2_peric", ENABLE_SCLK_PERIC, > 5, CLK_SET_RATE_PARENT, 0), > GATE(CLK_SCLK_SPI1, "sclk_spi1", "sclk_spi1_peric", ENABLE_SCLK_PERIC, > - 4, CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0), > + 4, CLK_SET_RATE_PARENT, 0), > GATE(CLK_SCLK_SPI0, "sclk_spi0", "sclk_spi0_peric", ENABLE_SCLK_PERIC, > 3, CLK_SET_RATE_PARENT, 0), > GATE(CLK_SCLK_UART2, "sclk_uart2", "sclk_uart2_peric", > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks 2016-07-07 12:46 ` Krzysztof Kozlowski @ 2016-07-07 13:27 ` Andi Shyti 2016-07-07 15:20 ` Sylwester Nawrocki 0 siblings, 1 reply; 9+ messages in thread From: Andi Shyti @ 2016-07-07 13:27 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Chanwoo Choi, Jaehoon Chung, Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti Hi Krzysztof, > > The CLK_IGNORE_UNUSED flag has to be avoided whenever possible. > > Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks, > > which enables the clock line during boot time. > > I don't agree. Both flags should be avoided. Clk is critical does not > solve the problem. It is just a better workaround for lack of proper > clock consumers. > > The IOCLK is not a critical clock. It can be disabled (e.g. when SoC is > used on a board without any SPI device connected). As we discussed offline there is no driver which is requesting this clock. We cannot ask to the spi driver to handle three clocks because the exynos5433 has its own peculiarities. If we want this on the spi driver's side, we need to add a new compatible and check it everytime. To me it looks uglier than just keep it alive. Thanks, Andi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks 2016-07-07 13:27 ` Andi Shyti @ 2016-07-07 15:20 ` Sylwester Nawrocki 2016-07-07 15:39 ` Andi Shyti 0 siblings, 1 reply; 9+ messages in thread From: Sylwester Nawrocki @ 2016-07-07 15:20 UTC (permalink / raw) To: Andi Shyti Cc: Krzysztof Kozlowski, Chanwoo Choi, Jaehoon Chung, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti On 07/07/2016 03:27 PM, Andi Shyti wrote: >>> > > The CLK_IGNORE_UNUSED flag has to be avoided whenever possible. >>> > > Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks, >>> > > which enables the clock line during boot time. >> > >> > I don't agree. Both flags should be avoided. Clk is critical does not >> > solve the problem. It is just a better workaround for lack of proper >> > clock consumers. >> > >> > The IOCLK is not a critical clock. It can be disabled (e.g. when SoC >> > is used on a board without any SPI device connected). > > As we discussed offline there is no driver which is requesting > this clock. We cannot ask to the spi driver to handle three > clocks because the exynos5433 has its own peculiarities. > > If we want this on the spi driver's side, we need to add a new > compatible and check it everytime. To me it looks uglier than > just keep it alive. I took a closer look at what those IOCLK clocks exactly are and unfortunately I must agree with Krzysztof. These clock gates are closely related to the IP blocks and to me proper approach is to have them listed in DT and controlled by the SPI bus driver. I checked and there is similar pattern for other IPs like I2S and other SoCs, e.g. exynos7420. Additionally parents of those IOCLK_SPI?_CLK clocks are currently wrongly modelled as fixed rate clocks. These clocks really don't have a parent until some clock is fed externally to the SoC's I/O pin. But this issue could be addressed later. I think it is not a big deal to add "samsung-exynos5433-spi" compatible to the SPI driver along with a new variant data and a flag like "has_cmu_ioclk" to indicate whether a third clock should be handled or not. Presumably for now the ioclk clock can just simply be enabled in probe(), this way it will be enabled only for SPI controllers actually in use. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks 2016-07-07 15:20 ` Sylwester Nawrocki @ 2016-07-07 15:39 ` Andi Shyti 0 siblings, 0 replies; 9+ messages in thread From: Andi Shyti @ 2016-07-07 15:39 UTC (permalink / raw) To: Sylwester Nawrocki Cc: Andi Shyti, Krzysztof Kozlowski, Chanwoo Choi, Jaehoon Chung, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti Hi Sylwester, > >>> > > The CLK_IGNORE_UNUSED flag has to be avoided whenever possible. > >>> > > Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks, > >>> > > which enables the clock line during boot time. > >> > > >> > I don't agree. Both flags should be avoided. Clk is critical does not > >> > solve the problem. It is just a better workaround for lack of proper > >> > clock consumers. > >> > > >> > The IOCLK is not a critical clock. It can be disabled (e.g. when SoC > >> > is used on a board without any SPI device connected). > > > > As we discussed offline there is no driver which is requesting > > this clock. We cannot ask to the spi driver to handle three > > clocks because the exynos5433 has its own peculiarities. > > > > If we want this on the spi driver's side, we need to add a new > > compatible and check it everytime. To me it looks uglier than > > just keep it alive. > > I took a closer look at what those IOCLK clocks exactly are and > unfortunately I must agree with Krzysztof. These clock gates are > closely related to the IP blocks and to me proper approach is to > have them listed in DT and controlled by the SPI bus driver. > > I checked and there is similar pattern for other IPs like I2S and > other SoCs, e.g. exynos7420. > Additionally parents of those IOCLK_SPI?_CLK clocks are currently > wrongly modelled as fixed rate clocks. These clocks really don't > have a parent until some clock is fed externally to the SoC's I/O > pin. But this issue could be addressed later. yes, it's a weird design :/ > I think it is not a big deal to add "samsung-exynos5433-spi" > compatible to the SPI driver along with a new variant data and > a flag like "has_cmu_ioclk" to indicate whether a third clock > should be handled or not. Presumably for now the ioclk clock can > just simply be enabled in probe(), this way it will be enabled > only for SPI controllers actually in use. OK, I will work on s3c64xx driver side, then. Thanks, Andi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 2/2] clk: exynos5433: enable sclk_ioclk for SPI3 2016-07-07 12:13 [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Andi Shyti 2016-07-07 12:13 ` [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks Andi Shyti @ 2016-07-07 12:13 ` Andi Shyti 2016-07-07 13:18 ` [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Sylwester Nawrocki 2 siblings, 0 replies; 9+ messages in thread From: Andi Shyti @ 2016-07-07 12:13 UTC (permalink / raw) To: Chanwoo Choi Cc: Jaehoon Chung, Sylwester Nawrocki, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti, Andi Shyti enable SPI3 critical clocks by using the CLK_IS_CRITICAL flag. There is no device which is supposed to enable this clock when needed, therefore, the only way to use the SPI bus is to enable it in boot time. Suggested-by: Tomasz Figa <tomasz.figa@gmail.com> Signed-off-by: Andi Shyti <andi.shyti@samsung.com> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/clk/samsung/clk-exynos5433.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c index 337387b..fb8d330 100644 --- a/drivers/clk/samsung/clk-exynos5433.c +++ b/drivers/clk/samsung/clk-exynos5433.c @@ -1648,7 +1648,8 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = { GATE(CLK_SCLK_IOCLK_SPI4, "sclk_ioclk_spi4", "ioclk_spi4_clk_in", ENABLE_SCLK_PERIC, 21, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_IOCLK_SPI3, "sclk_ioclk_spi3", "ioclk_spi3_clk_in", - ENABLE_SCLK_PERIC, 20, CLK_SET_RATE_PARENT, 0), + ENABLE_SCLK_PERIC, 20, + CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_SPI4, "sclk_spi4", "sclk_spi4_peric", ENABLE_SCLK_PERIC, 19, CLK_SET_RATE_PARENT, 0), GATE(CLK_SCLK_SPI3, "sclk_spi3", "sclk_spi3_peric", ENABLE_SCLK_PERIC, -- 2.8.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks 2016-07-07 12:13 [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Andi Shyti 2016-07-07 12:13 ` [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks Andi Shyti 2016-07-07 12:13 ` [PATCH v4 2/2] clk: exynos5433: enable sclk_ioclk for SPI3 Andi Shyti @ 2016-07-07 13:18 ` Sylwester Nawrocki 2016-07-07 15:26 ` Sylwester Nawrocki 2 siblings, 1 reply; 9+ messages in thread From: Sylwester Nawrocki @ 2016-07-07 13:18 UTC (permalink / raw) To: Andi Shyti Cc: Chanwoo Choi, Jaehoon Chung, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti On 07/07/2016 02:13 PM, Andi Shyti wrote: > Andi Shyti (2): > clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks > clk: exynos5433: enable sclk_ioclk for SPI3 Applied both patches, thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks 2016-07-07 13:18 ` [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Sylwester Nawrocki @ 2016-07-07 15:26 ` Sylwester Nawrocki 0 siblings, 0 replies; 9+ messages in thread From: Sylwester Nawrocki @ 2016-07-07 15:26 UTC (permalink / raw) To: Andi Shyti Cc: Chanwoo Choi, Jaehoon Chung, Tomasz Figa, Michael Turquette, Stephen Boyd, Kukjin Kim, Krzysztof Kozlowski, linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel, Andi Shyti On 07/07/2016 03:18 PM, Sylwester Nawrocki wrote: > On 07/07/2016 02:13 PM, Andi Shyti wrote: >> > Andi Shyti (2): >> > clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks >> > clk: exynos5433: enable sclk_ioclk for SPI3 > > Applied both patches, thanks. Apologies for the confusion, I had to drop these patches for reasons explained in the other thread. We just need to simply drop CLK_IGNORE_UNUSED flags and get the driver controlling clocks which belong to the peripheral IP block. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-07 15:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-07 12:13 [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Andi Shyti 2016-07-07 12:13 ` [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks Andi Shyti 2016-07-07 12:46 ` Krzysztof Kozlowski 2016-07-07 13:27 ` Andi Shyti 2016-07-07 15:20 ` Sylwester Nawrocki 2016-07-07 15:39 ` Andi Shyti 2016-07-07 12:13 ` [PATCH v4 2/2] clk: exynos5433: enable sclk_ioclk for SPI3 Andi Shyti 2016-07-07 13:18 ` [PATCH v4 0/2] mark spi clocks as critical and enable spi3 clocks Sylwester Nawrocki 2016-07-07 15:26 ` Sylwester Nawrocki
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).