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

2016-07-06 Thread Sylwester Nawrocki 
On 07/06/2016 06:51 AM, Andi Shyti wrote:
> 
> I don't see anything wrong on the above. We could make it as:

> @@ -640,13 +639,7 @@ static void s3c64xx_spi_config(struct 
> s3c64xx_spi_driver_data *sdd)
>  
> writel(val, regs + S3C64XX_SPI_MODE_CFG);
>  
> -   if (sdd->port_conf->clk_from_cmu) {
> -   /* Configure Clock */
> -   /* There is half-multiplier before the SPI */
> -   clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> -   /* Enable Clock */
> -   clk_prepare_enable(sdd->src_clk);

clk_set_rate() call needs to stay, we can only remove clk_prepare_enable().

> -   } else {
> +   if (!sdd->port_conf->clk_from_cmu) {
> /* Configure Clock */
> val = readl(regs + S3C64XX_SPI_CLK_CFG);
> val &= ~S3C64XX_SPI_PSR_MASK;
> 
>> I meant we could amend which clocks are specified at the SPI bus device
>> DT nodes and change handling of clocks in the spi-s3c64xx driver to model
>> everything properly and get it all working.
> 
> I think that if the clock comes from the cmu it's not necessary
> to disable it. I would like to avoid adding DTS properties
> because we have the clock disabling inherited from old code which
> it might not be required at all (in our tests, indeed it works).

OK, anyway we already need to amend exynos5433.dtsi file to change
the "spi_busclk0" clock specifier.

I agree we can get rid of the clock gating in s3c64xx_spi_config() 
function, it should not do any harm and will help in getting rid 
of the bus access exceptions.

In general PCLK should be enough for accessing register of the controller,
one hypothesis is that automatic clock gating may be disabling PCLK
when it sees SCLK inactive.

I tested on exynos4412 trats2 board a large firmware file upload over 
SPI and didn't notice any bad side effects with above 
clk_disable_unprepare()/clk_prepare_enable() calls commented out.

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

2016-07-06 Thread Sylwester Nawrocki 
On 07/06/2016 06:51 AM, Andi Shyti wrote:
> 
> I don't see anything wrong on the above. We could make it as:

> @@ -640,13 +639,7 @@ static void s3c64xx_spi_config(struct 
> s3c64xx_spi_driver_data *sdd)
>  
> writel(val, regs + S3C64XX_SPI_MODE_CFG);
>  
> -   if (sdd->port_conf->clk_from_cmu) {
> -   /* Configure Clock */
> -   /* There is half-multiplier before the SPI */
> -   clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> -   /* Enable Clock */
> -   clk_prepare_enable(sdd->src_clk);

clk_set_rate() call needs to stay, we can only remove clk_prepare_enable().

> -   } else {
> +   if (!sdd->port_conf->clk_from_cmu) {
> /* Configure Clock */
> val = readl(regs + S3C64XX_SPI_CLK_CFG);
> val &= ~S3C64XX_SPI_PSR_MASK;
> 
>> I meant we could amend which clocks are specified at the SPI bus device
>> DT nodes and change handling of clocks in the spi-s3c64xx driver to model
>> everything properly and get it all working.
> 
> I think that if the clock comes from the cmu it's not necessary
> to disable it. I would like to avoid adding DTS properties
> because we have the clock disabling inherited from old code which
> it might not be required at all (in our tests, indeed it works).

OK, anyway we already need to amend exynos5433.dtsi file to change
the "spi_busclk0" clock specifier.

I agree we can get rid of the clock gating in s3c64xx_spi_config() 
function, it should not do any harm and will help in getting rid 
of the bus access exceptions.

In general PCLK should be enough for accessing register of the controller,
one hypothesis is that automatic clock gating may be disabling PCLK
when it sees SCLK inactive.

I tested on exynos4412 trats2 board a large firmware file upload over 
SPI and didn't notice any bad side effects with above 
clk_disable_unprepare()/clk_prepare_enable() calls commented out.

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

2016-07-05 Thread Andi Shyti 
Hi Sylwester,

> +#if 0
> clocks = <_peric CLK_PCLK_SPI1>,
>  <_top CLK_SCLK_SPI1_PERIC>;
> +#else
> +   clocks = <_peric CLK_PCLK_SPI1>,
> +<_peric CLK_SCLK_SPI1>;
> +#endif

Yes, that's how it should be, indeed.

> /* Disable Clock */
> if (sdd->port_conf->clk_from_cmu) {
> -   clk_disable_unprepare(sdd->src_clk);
> +   /* clk_disable_unprepare(sdd->src_clk); */
> } else {
> val = readl(regs + S3C64XX_SPI_CLK_CFG);
> val &= ~S3C64XX_SPI_ENCLK_ENABLE;
> @@ -626,7 +626,7 @@ static void s3c64xx_spi_config(struct 
> s3c64xx_spi_driver_data *sdd)
> /* There is half-multiplier before the SPI */
> clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> /* Enable Clock */
> -   clk_prepare_enable(sdd->src_clk);
> +   /* clk_prepare_enable(sdd->src_clk); */
> } else {
> /* Configure Clock */
> val = readl(regs + S3C64XX_SPI_CLK_CFG);

I don't see anything wrong on the above. We could make it as:

@@ -596,9 +597,7 @@ static void s3c64xx_spi_config(struct 
s3c64xx_spi_driver_data *sdd)
u32 val;
 
/* Disable Clock */
-   if (sdd->port_conf->clk_from_cmu) {
-   clk_disable_unprepare(sdd->src_clk);
-   } else {
+   if (!sdd->port_conf->clk_from_cmu) {
val = readl(regs + S3C64XX_SPI_CLK_CFG);
val &= ~S3C64XX_SPI_ENCLK_ENABLE;
writel(val, regs + S3C64XX_SPI_CLK_CFG);
@@ -640,13 +639,7 @@ static void s3c64xx_spi_config(struct 
s3c64xx_spi_driver_data *sdd)
 
writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
-   if (sdd->port_conf->clk_from_cmu) {
-   /* Configure Clock */
-   /* There is half-multiplier before the SPI */
-   clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
-   /* Enable Clock */
-   clk_prepare_enable(sdd->src_clk);
-   } else {
+   if (!sdd->port_conf->clk_from_cmu) {
/* Configure Clock */
val = readl(regs + S3C64XX_SPI_CLK_CFG);
val &= ~S3C64XX_SPI_PSR_MASK;

> I meant we could amend which clocks are specified at the SPI bus device
> DT nodes and change handling of clocks in the spi-s3c64xx driver to model
> everything properly and get it all working.

I think that if the clock comes from the cmu it's not necessary
to disable it. I would like to avoid adding DTS properties
because we have the clock disabling inherited from old code which
it might not be required at all (in our tests, indeed it works).

Thanks,
Andi

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

2016-07-05 Thread Andi Shyti 
Hi Sylwester,

> +#if 0
> clocks = <_peric CLK_PCLK_SPI1>,
>  <_top CLK_SCLK_SPI1_PERIC>;
> +#else
> +   clocks = <_peric CLK_PCLK_SPI1>,
> +<_peric CLK_SCLK_SPI1>;
> +#endif

Yes, that's how it should be, indeed.

> /* Disable Clock */
> if (sdd->port_conf->clk_from_cmu) {
> -   clk_disable_unprepare(sdd->src_clk);
> +   /* clk_disable_unprepare(sdd->src_clk); */
> } else {
> val = readl(regs + S3C64XX_SPI_CLK_CFG);
> val &= ~S3C64XX_SPI_ENCLK_ENABLE;
> @@ -626,7 +626,7 @@ static void s3c64xx_spi_config(struct 
> s3c64xx_spi_driver_data *sdd)
> /* There is half-multiplier before the SPI */
> clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
> /* Enable Clock */
> -   clk_prepare_enable(sdd->src_clk);
> +   /* clk_prepare_enable(sdd->src_clk); */
> } else {
> /* Configure Clock */
> val = readl(regs + S3C64XX_SPI_CLK_CFG);

I don't see anything wrong on the above. We could make it as:

@@ -596,9 +597,7 @@ static void s3c64xx_spi_config(struct 
s3c64xx_spi_driver_data *sdd)
u32 val;
 
/* Disable Clock */
-   if (sdd->port_conf->clk_from_cmu) {
-   clk_disable_unprepare(sdd->src_clk);
-   } else {
+   if (!sdd->port_conf->clk_from_cmu) {
val = readl(regs + S3C64XX_SPI_CLK_CFG);
val &= ~S3C64XX_SPI_ENCLK_ENABLE;
writel(val, regs + S3C64XX_SPI_CLK_CFG);
@@ -640,13 +639,7 @@ static void s3c64xx_spi_config(struct 
s3c64xx_spi_driver_data *sdd)
 
writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
-   if (sdd->port_conf->clk_from_cmu) {
-   /* Configure Clock */
-   /* There is half-multiplier before the SPI */
-   clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
-   /* Enable Clock */
-   clk_prepare_enable(sdd->src_clk);
-   } else {
+   if (!sdd->port_conf->clk_from_cmu) {
/* Configure Clock */
val = readl(regs + S3C64XX_SPI_CLK_CFG);
val &= ~S3C64XX_SPI_PSR_MASK;

> I meant we could amend which clocks are specified at the SPI bus device
> DT nodes and change handling of clocks in the spi-s3c64xx driver to model
> everything properly and get it all working.

I think that if the clock comes from the cmu it's not necessary
to disable it. I would like to avoid adding DTS properties
because we have the clock disabling inherited from old code which
it might not be required at all (in our tests, indeed it works).

Thanks,
Andi

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

2016-07-04 Thread Sylwester Nawrocki 
On 07/04/2016 12:26 PM, Andi Shyti wrote:
 
> The single clock lines are not configured in the exynos5433 dts,
> but in the drivers/clk/samsung/clk-exynos5433.c file and it's the
> only place where we can set the flags.

I meant we could amend which clocks are specified at the SPI bus device
DT nodes and change handling of clocks in the spi-s3c64xx driver to model
everything properly and get it all working.
 
>> What is an exact problem here, are you perhaps testing suspend to RAM?
>> I tested my sound support patches on top of v4.7-rc1 and everything
>> seemed to work well, I didn't notice any issues with the audio codec
>> which was the only slave on the SPI 1 bus.
> 
> Yes, because the audio codec is on SPI1 and its bus line
> (spi_busclk0) is CLK_SCLK_SPI1_PERIC while the CLK_SCLK_SPI1 is
> set as CLK_IGNORE_UNUSED.

That's true, looking at a downstream kernel I see that there is just plain 
div clock specified for spi_busclk0 (DIVsclk_spi1_b), i.e. SCLK_SPI1_PERIC 
and SCLK_SPI1 don't get disabled in s3c64xx_spi_config().
It seems SCLK_SPI1 in CMU_PERIC need to be kept enabled while accessing 
the SPI controller's registers.

>> Doesn't it help when you specify CLK_SCLK_SPI1 as the second clock
>> ("spi_busclk0") of the spi_1 bus controller instead of
>> CLK_SCLK_SPI0_PERIC? CLK_SCLK_SPI0_PERIC seem to be parent of
>> CLK_SCLK_SPI1 so the enable state would be propagated.
> 
> nope! :(
> 
> For some reasons, if you set in the DTS as spi_busclk0 the
> CLK_SCLK_SPI1 from cmu_peric you get a synchronus abort in the
> s3c64xx_spi_config (the first read performed on the device).

Indeed, I also observed that, after removing CLK_IGNORE_UNUSED from 
the CLK_SCLK_SPI1 clock. 

After discussion with Krzysztof and Andrzej I came up with a patch as below 
where there is no aborts, the sound works and clocks are not kept always 
enabled:

root@localhost:~# cat /sys/kernel/debug/clk/clk_summary | grep spi1
 ioclk_spi1_clk_in005000  0 
0  
sclk_ioclk_spi1   005000  0 
0  
  pclk_isp_spi1   00 600  0 
0  
mout_sclk_isp_spi1_user   002400  0 
0  
   sclk_isp_spi1  002400  0 
0  
mout_sclk_spi1002400  0 
0  
   div_sclk_spi1_a00 300  0 
0  
  div_sclk_spi1_b 00 300  0 
0  
 sclk_spi1_peric  00 300  0 
0  
sclk_spi1 00 300  0 
0  
mout_sclk_isp_spi1002400  0 
0  
   div_sclk_isp_spi1_a00 300  0 
0  
  div_sclk_isp_spi1_b 00   25000  0 
0  
 sclk_isp_spi1_cam1   00   25000  0 
0  
  pclk_spi1   006667  0 
0  

I'm not yet 100% sure if it is a correct approach, the downstream kernel uses
"global-per-IP" gate clocks (ENABLE_IP_PERIC? registers), that gate all clocks 
to a given IP block and those clocks are not defined in mainline at all, but 
it seems we just need to amend the SPI controller driver to not be disabling
sdd->src_clk clock before accessing registers. 
Or maybe to pass only DIVsclk_spi?_b as spi_busclk0 in DT nodes and add 
SCLK_SPI0 from CMU_PERIC as a third SPI device clock for exynos5433.

---8<
diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index 8e124fc..f444c66 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -617,8 +617,13 @@
dma-names = "tx", "rx";
#address-cells = <1>;
#size-cells = <0>;
+#if 0
clocks = <_peric CLK_PCLK_SPI1>,
 <_top CLK_SCLK_SPI1_PERIC>;
+#else
+   clocks = <_peric CLK_PCLK_SPI1>,
+<_peric CLK_SCLK_SPI1>;
+#endif
clock-names = "spi", "spi_busclk0";
samsung,spi-src-clk = <0>;
pinctrl-names = "default";
diff --git a/drivers/clk/samsung/clk-exynos5433.c 
b/drivers/clk/samsung/clk-exynos5433.c
index e3cc935..61d5643 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -1675,7 +1675,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,

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

2016-07-04 Thread Sylwester Nawrocki 
On 07/04/2016 12:26 PM, Andi Shyti wrote:
 
> The single clock lines are not configured in the exynos5433 dts,
> but in the drivers/clk/samsung/clk-exynos5433.c file and it's the
> only place where we can set the flags.

I meant we could amend which clocks are specified at the SPI bus device
DT nodes and change handling of clocks in the spi-s3c64xx driver to model
everything properly and get it all working.
 
>> What is an exact problem here, are you perhaps testing suspend to RAM?
>> I tested my sound support patches on top of v4.7-rc1 and everything
>> seemed to work well, I didn't notice any issues with the audio codec
>> which was the only slave on the SPI 1 bus.
> 
> Yes, because the audio codec is on SPI1 and its bus line
> (spi_busclk0) is CLK_SCLK_SPI1_PERIC while the CLK_SCLK_SPI1 is
> set as CLK_IGNORE_UNUSED.

That's true, looking at a downstream kernel I see that there is just plain 
div clock specified for spi_busclk0 (DIVsclk_spi1_b), i.e. SCLK_SPI1_PERIC 
and SCLK_SPI1 don't get disabled in s3c64xx_spi_config().
It seems SCLK_SPI1 in CMU_PERIC need to be kept enabled while accessing 
the SPI controller's registers.

>> Doesn't it help when you specify CLK_SCLK_SPI1 as the second clock
>> ("spi_busclk0") of the spi_1 bus controller instead of
>> CLK_SCLK_SPI0_PERIC? CLK_SCLK_SPI0_PERIC seem to be parent of
>> CLK_SCLK_SPI1 so the enable state would be propagated.
> 
> nope! :(
> 
> For some reasons, if you set in the DTS as spi_busclk0 the
> CLK_SCLK_SPI1 from cmu_peric you get a synchronus abort in the
> s3c64xx_spi_config (the first read performed on the device).

Indeed, I also observed that, after removing CLK_IGNORE_UNUSED from 
the CLK_SCLK_SPI1 clock. 

After discussion with Krzysztof and Andrzej I came up with a patch as below 
where there is no aborts, the sound works and clocks are not kept always 
enabled:

root@localhost:~# cat /sys/kernel/debug/clk/clk_summary | grep spi1
 ioclk_spi1_clk_in005000  0 
0  
sclk_ioclk_spi1   005000  0 
0  
  pclk_isp_spi1   00 600  0 
0  
mout_sclk_isp_spi1_user   002400  0 
0  
   sclk_isp_spi1  002400  0 
0  
mout_sclk_spi1002400  0 
0  
   div_sclk_spi1_a00 300  0 
0  
  div_sclk_spi1_b 00 300  0 
0  
 sclk_spi1_peric  00 300  0 
0  
sclk_spi1 00 300  0 
0  
mout_sclk_isp_spi1002400  0 
0  
   div_sclk_isp_spi1_a00 300  0 
0  
  div_sclk_isp_spi1_b 00   25000  0 
0  
 sclk_isp_spi1_cam1   00   25000  0 
0  
  pclk_spi1   006667  0 
0  

I'm not yet 100% sure if it is a correct approach, the downstream kernel uses
"global-per-IP" gate clocks (ENABLE_IP_PERIC? registers), that gate all clocks 
to a given IP block and those clocks are not defined in mainline at all, but 
it seems we just need to amend the SPI controller driver to not be disabling
sdd->src_clk clock before accessing registers. 
Or maybe to pass only DIVsclk_spi?_b as spi_busclk0 in DT nodes and add 
SCLK_SPI0 from CMU_PERIC as a third SPI device clock for exynos5433.

---8<
diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
index 8e124fc..f444c66 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
@@ -617,8 +617,13 @@
dma-names = "tx", "rx";
#address-cells = <1>;
#size-cells = <0>;
+#if 0
clocks = <_peric CLK_PCLK_SPI1>,
 <_top CLK_SCLK_SPI1_PERIC>;
+#else
+   clocks = <_peric CLK_PCLK_SPI1>,
+<_peric CLK_SCLK_SPI1>;
+#endif
clock-names = "spi", "spi_busclk0";
samsung,spi-src-clk = <0>;
pinctrl-names = "default";
diff --git a/drivers/clk/samsung/clk-exynos5433.c 
b/drivers/clk/samsung/clk-exynos5433.c
index e3cc935..61d5643 100644
--- a/drivers/clk/samsung/clk-exynos5433.c
+++ b/drivers/clk/samsung/clk-exynos5433.c
@@ -1675,7 +1675,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,

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

2016-07-04 Thread Andi Shyti 
Hi Sylwester,

> > GATE(CLK_SCLK_SPI1, "sclk_spi1", "sclk_spi1_peric", ENABLE_SCLK_PERIC,
> > -   4, CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0),
> > +   4, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),
> 
> As Tomasz pointed out, this should be addressed in the driver/dts,
> we shouldn't be patching board configurations into a per-SoC driver.
> Other boards may want to keep this clock disabled.

The single clock lines are not configured in the exynos5433 dts,
but in the drivers/clk/samsung/clk-exynos5433.c file and it's the
only place where we can set the flags.

> What is an exact problem here, are you perhaps testing suspend to RAM?
> I tested my sound support patches on top of v4.7-rc1 and everything
> seemed to work well, I didn't notice any issues with the audio codec
> which was the only slave on the SPI 1 bus.

Yes, because the audio codec is on SPI1 and its bus line
(spi_busclk0) is CLK_SCLK_SPI1_PERIC while the CLK_SCLK_SPI1 is
set as CLK_IGNORE_UNUSED.

> Doesn't it help when you specify CLK_SCLK_SPI1 as the second clock
> ("spi_busclk0") of the spi_1 bus controller instead of
> CLK_SCLK_SPI0_PERIC? CLK_SCLK_SPI0_PERIC seem to be parent of
> CLK_SCLK_SPI1 so the enable state would be propagated.

nope! :(

For some reasons, if you set in the DTS as spi_busclk0 the
CLK_SCLK_SPI1 from cmu_peric you get a synchronus abort in the
s3c64xx_spi_config (the first read performed on the device).

Andi

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

2016-07-04 Thread Andi Shyti 
Hi Sylwester,

> > GATE(CLK_SCLK_SPI1, "sclk_spi1", "sclk_spi1_peric", ENABLE_SCLK_PERIC,
> > -   4, CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0),
> > +   4, CLK_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),
> 
> As Tomasz pointed out, this should be addressed in the driver/dts,
> we shouldn't be patching board configurations into a per-SoC driver.
> Other boards may want to keep this clock disabled.

The single clock lines are not configured in the exynos5433 dts,
but in the drivers/clk/samsung/clk-exynos5433.c file and it's the
only place where we can set the flags.

> What is an exact problem here, are you perhaps testing suspend to RAM?
> I tested my sound support patches on top of v4.7-rc1 and everything
> seemed to work well, I didn't notice any issues with the audio codec
> which was the only slave on the SPI 1 bus.

Yes, because the audio codec is on SPI1 and its bus line
(spi_busclk0) is CLK_SCLK_SPI1_PERIC while the CLK_SCLK_SPI1 is
set as CLK_IGNORE_UNUSED.

> Doesn't it help when you specify CLK_SCLK_SPI1 as the second clock
> ("spi_busclk0") of the spi_1 bus controller instead of
> CLK_SCLK_SPI0_PERIC? CLK_SCLK_SPI0_PERIC seem to be parent of
> CLK_SCLK_SPI1 so the enable state would be propagated.

nope! :(

For some reasons, if you set in the DTS as spi_busclk0 the
CLK_SCLK_SPI1 from cmu_peric you get a synchronus abort in the
s3c64xx_spi_config (the first read performed on the device).

Andi

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

2016-07-04 Thread Sylwester Nawrocki 
On 06/30/2016 09:15 AM, Andi Shyti wrote:
> The CLK_IGNORE_UNUSED flag has to be avoided whenever possible.

In general I would rather disagree.

> Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks,
> which enables the clock line during boot time.
> 
> Suggested-by: Tomasz Figa 
> Signed-off-by: Andi Shyti 
> ---
>  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..1f7c4951 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 = {

> @@ -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_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),

As Tomasz pointed out, this should be addressed in the driver/dts,
we shouldn't be patching board configurations into a per-SoC driver.
Other boards may want to keep this clock disabled.

What is an exact problem here, are you perhaps testing suspend to RAM?
I tested my sound support patches on top of v4.7-rc1 and everything
seemed to work well, I didn't notice any issues with the audio codec
which was the only slave on the SPI 1 bus.

Doesn't it help when you specify CLK_SCLK_SPI1 as the second clock
("spi_busclk0") of the spi_1 bus controller instead of
CLK_SCLK_SPI0_PERIC? CLK_SCLK_SPI0_PERIC seem to be parent of
CLK_SCLK_SPI1 so the enable state would be propagated.

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

2016-07-04 Thread Sylwester Nawrocki 
On 06/30/2016 09:15 AM, Andi Shyti wrote:
> The CLK_IGNORE_UNUSED flag has to be avoided whenever possible.

In general I would rather disagree.

> Use the CLK_IS_CRITICAL flag instead for critical SPI1 clocks,
> which enables the clock line during boot time.
> 
> Suggested-by: Tomasz Figa 
> Signed-off-by: Andi Shyti 
> ---
>  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..1f7c4951 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 = {

> @@ -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_IS_CRITICAL | CLK_SET_RATE_PARENT, 0),

As Tomasz pointed out, this should be addressed in the driver/dts,
we shouldn't be patching board configurations into a per-SoC driver.
Other boards may want to keep this clock disabled.

What is an exact problem here, are you perhaps testing suspend to RAM?
I tested my sound support patches on top of v4.7-rc1 and everything
seemed to work well, I didn't notice any issues with the audio codec
which was the only slave on the SPI 1 bus.

Doesn't it help when you specify CLK_SCLK_SPI1 as the second clock
("spi_busclk0") of the spi_1 bus controller instead of
CLK_SCLK_SPI0_PERIC? CLK_SCLK_SPI0_PERIC seem to be parent of
CLK_SCLK_SPI1 so the enable state would be propagated.

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

2016-06-30 Thread 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 
Signed-off-by: Andi Shyti 
---
 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..1f7c4951 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_IS_CRITICAL | 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

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

2016-06-30 Thread 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 
Signed-off-by: Andi Shyti 
---
 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..1f7c4951 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_IS_CRITICAL | 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