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

Groups > linux.kernel > #1434289 > unrolled thread

[PATCH v3 0/2] mark spi clocks as critical and enable spi3 clocks

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/2] mark spi clocks as critical and enable spi3 clocks Andi Shyti <andi.shyti@samsung.com> - 2016-06-30 09:30 +0200
    [PATCH v3 2/2] clk: exynos5433: enable sclk_ioclk for SPI3 Andi Shyti <andi.shyti@samsung.com> - 2016-06-30 09:30 +0200
    Re: [PATCH v3 0/2] mark spi clocks as critical and enable spi3 clocks Tomasz Figa <tomasz.figa@gmail.com> - 2016-07-04 06:10 +0200
      Re: [PATCH v3 0/2] mark spi clocks as critical and enable spi3 clocks Andi Shyti <andi.shyti@samsung.com> - 2016-07-04 06:30 +0200
        Re: [PATCH v3 0/2] mark spi clocks as critical and enable spi3 clocks Tomasz Figa <tomasz.figa@gmail.com> - 2016-07-04 07:00 +0200

#1434289 — [PATCH v3 0/2] mark spi clocks as critical and enable spi3 clocks

Hi,

This patchset enables spi3 clocks by marking critical clocks as
CLK_IS_CRITICAL and in the meantime replaces the use of
CLK_IGNORE_UNUSED for spi1 clocks.

The sclk needs to be considered critical as well.

The SPI3 in the tm2/tm2e boards will control an irled for remote
controlling.

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.

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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

-- 
2.8.1

[toc] | [next] | [standalone]

#1434295 — [PATCH v3 2/2] clk: exynos5433: enable sclk_ioclk for SPI3

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5433.c b/drivers/clk/samsung/clk-exynos5433.c
index 1f7c4951..e769c80 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -1648,11 +1648,12 @@ 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,
-			18, CLK_SET_RATE_PARENT, 0),
+			18, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_SCI, "sclk_sci", "div_sclk_sci", ENABLE_SCLK_PERIC,
 			17, 0, 0),
 	GATE(CLK_SCLK_SC_IN, "sclk_sc_in", "div_sclk_sc_in", ENABLE_SCLK_PERIC,
-- 
2.8.1

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

#1436407

Hi Andi,

2016-06-30 16:15 GMT+09:00 Andi Shyti <andi.shyti@samsung.com>:
> Hi,
>
> This patchset enables spi3 clocks by marking critical clocks as
> CLK_IS_CRITICAL and in the meantime replaces the use of
> CLK_IGNORE_UNUSED for spi1 clocks.
>
> The sclk needs to be considered critical as well.
>
> The SPI3 in the tm2/tm2e boards will control an irled for remote
> controlling.
>
> 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!

This sounds like a DTS and/or SPI and/or clock driver bug then. I'm
pretty much convinced that it might also cause other problems and this
workaround will only mask them making things even more difficult to
debug in the future. Let's not introduce more technical debt.

Best regards,
Tomasz

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

#1436436

Hi Tomasz,

> > This patchset enables spi3 clocks by marking critical clocks as
> > CLK_IS_CRITICAL and in the meantime replaces the use of
> > CLK_IGNORE_UNUSED for spi1 clocks.
> >
> > The sclk needs to be considered critical as well.
> >
> > The SPI3 in the tm2/tm2e boards will control an irled for remote
> > controlling.
> >
> > 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!
> 
> This sounds like a DTS and/or SPI and/or clock driver bug then. I'm
> pretty much convinced that it might also cause other problems and this
> workaround will only mask them making things even more difficult to
> debug in the future. Let's not introduce more technical debt.

In principle I agree, but, the upcoming TM2/TM2e boards will have
the audio codec connected on SPI1 and the ir remote controller on
the SPI3 (others SPI slots are used as well, but we don't have
the drivers ready for them).

I followed for SPI3 the same approach as it was done for SPI1, so
that I see two choices here:

 * reject this patches and cleanup the SPI1 from the "technical
   debt" and not have anything working on spi.

or

 * do for SPI3 the same as it has been done for SPI1 (and as it
   is now) with its "technical debt".

I did quite an extensive testing on these lines and I don't see
any other alternative.

Thanks,
Andi

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

#1436505

2016-07-04 13:20 GMT+09:00 Andi Shyti <andi.shyti@samsung.com>:
> Hi Tomasz,
>
>> > This patchset enables spi3 clocks by marking critical clocks as
>> > CLK_IS_CRITICAL and in the meantime replaces the use of
>> > CLK_IGNORE_UNUSED for spi1 clocks.
>> >
>> > The sclk needs to be considered critical as well.
>> >
>> > The SPI3 in the tm2/tm2e boards will control an irled for remote
>> > controlling.
>> >
>> > 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!
>>
>> This sounds like a DTS and/or SPI and/or clock driver bug then. I'm
>> pretty much convinced that it might also cause other problems and this
>> workaround will only mask them making things even more difficult to
>> debug in the future. Let's not introduce more technical debt.
>
> In principle I agree, but, the upcoming TM2/TM2e boards will have
> the audio codec connected on SPI1 and the ir remote controller on
> the SPI3 (others SPI slots are used as well, but we don't have
> the drivers ready for them).
>
> I followed for SPI3 the same approach as it was done for SPI1, so
> that I see two choices here:
>
>  * reject this patches and cleanup the SPI1 from the "technical
>    debt" and not have anything working on spi.

>
> or
>
>  * do for SPI3 the same as it has been done for SPI1 (and as it
>    is now) with its "technical debt".
>
> I did quite an extensive testing on these lines and I don't see
> any other alternative.

The SPI driver is supposed to enable the SPI bus clock whenever
necessary, if it's given the clock correctly and the clock driver has
the clock hierarchy correctly defined. So I see third choice here:

 * Debug why the SPI driver doesn't properly enable the clock, fix the
cause (fixing both SPI1 and SPI3 sclk at the same time) and then
remove the CLK_IGNORE_UNUSED flag from sclk_spi1.

Of course the above doesn't apply to sclk_ioclk clocks, as we already
agreed that they need CLK_IS_CRITICAL due to their nature.

Best regards,
Tomasz

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web