Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1438551 > unrolled thread

[PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI  clocks Andi Shyti <andi.shyti@samsung.com> - 2016-07-07 14:20 +0200
    Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for  SPI clocks Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-07-07 14:50 +0200
      Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for  SPI clocks Andi Shyti <andi.shyti@samsung.com> - 2016-07-07 15:30 +0200
        Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for  SPI clocks Sylwester Nawrocki <s.nawrocki@samsung.com> - 2016-07-07 17:30 +0200
          Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for  SPI clocks Andi Shyti <andi@etezian.org> - 2016-07-07 17:40 +0200

#1438551 — [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks

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

[toc] | [next] | [standalone]

#1438584 — Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks

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

[toc] | [prev] | [next] | [standalone]

#1438624 — Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks

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

[toc] | [prev] | [next] | [standalone]

#1438677 — Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks

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.

[toc] | [prev] | [next] | [standalone]

#1438685 — Re: [PATCH v4 1/2] clk: exynos5433: do not use CLK_IGNORE_UNUSED for SPI clocks

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web