[v3,0/5] Fix problems with Exynos4412 ISP clocks

Message ID 20171011092515.1698-1-m.szyprowski@samsung.com
Headers show
Series
  • Fix problems with Exynos4412 ISP clocks
Related show

Message

Marek Szyprowski Oct. 11, 2017, 9:25 a.m.
Hi!

Exynos4412 ISP clock controller is located in the SOC area, which belongs
to ISP power domain. This was not properly handled by the current
Exynos4-clk driver. This patchset instantiates a separate clock driver
for those clocks, updates all clients of ISP clocks and ensures that
the driver is properly integrated in ISP power domin using runtime PM
feature of the clock framework.

This finally solves all the mysterious freezes in accessing ISP clocks
when ISP power domain is disabled.

The last patch breaks support for old dtbs. It can be applied when all
boards are updated. Exynos4412 ISP subsystem is only used by Trats2
boards, for which kernel is updated always together with the dtb file,
so the last patch can be applied to the next kernel release after merging
the DTS patch.

This patchset requires clocks runtime PM support ("Add runtime PM support
for clocks (on Exynos SoC example)" v9 patchset), which has been recently
merged to clk-next.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Changelog:
v3:
- moved dt-bindings changes to separate patch and rewrote dt bindings
  documentation as requrested by Rob Herring

v2: https://www.spinics.net/lists/linux-samsung-soc/msg60737.html
- fixed minor issues pointed by Krzysztof Kozlowski

v1: https://www.spinics.net/lists/linux-clk/msg20274.html
- initial version of this patchset

Patch summary:

Marek Szyprowski (5):
  clk: samsung: Instantiate Exynos4412 ISP clocks only when available
  clk: samsung: Add dt bindings for Exynos4412 ISP clock controller
  clk: samsung: Add a separate driver for Exynos4412 ISP clocks
  ARM: dts: exynos: Add Exynos4412 ISP clock controller
  clk: samsung: Remove obsolete code for Exynos4412 ISP clocks

 .../devicetree/bindings/clock/exynos4-clock.txt    |  43 +++++
 arch/arm/boot/dts/exynos4412.dtsi                  |  71 ++++----
 drivers/clk/samsung/Makefile                       |   1 +
 drivers/clk/samsung/clk-exynos4.c                  |  66 +-------
 drivers/clk/samsung/clk-exynos4412-isp.c           | 179 +++++++++++++++++++++
 include/dt-bindings/clock/exynos4.h                |  65 ++++----
 6 files changed, 303 insertions(+), 122 deletions(-)
 create mode 100644 drivers/clk/samsung/clk-exynos4412-isp.c

-- 
2.14.2

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Krzysztof Kozlowski Oct. 11, 2017, 5:05 p.m. | #1
On Wed, Oct 11, 2017 at 11:25:12AM +0200, Marek Szyprowski wrote:
> Some registers for the Exynos 4412 ISP (Camera subsystem) clocks are

> located in the ISP power domain. Because those registers are also

> located in a different memory region than the main clock controller,

> support for them can be provided by a separate clock controller.

> 

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  .../devicetree/bindings/clock/exynos4-clock.txt    | 43 ++++++++++++++++++++++

>  include/dt-bindings/clock/exynos4.h                | 35 ++++++++++++++++++

>  2 files changed, 78 insertions(+)

> 

> diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt b/Documentation/devicetree/bindings/clock/exynos4-clock.txt

> index f5a5b19ed3b2..bc61c952cb0b 100644

> --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt

> +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt

> @@ -41,3 +41,46 @@ Example 2: UART controller node that consumes the clock generated by the clock

>  		clocks = <&clock CLK_UART2>, <&clock CLK_SCLK_UART2>;

>  		clock-names = "uart", "clk_uart_baud0";

>  	};

> +

> +Exynos4412 SoC contains some additional clocks for FIMC-ISP (Camera ISP)

> +subsystem. Registers for those clocks are located in the ISP power domain.

> +Because those registers are also located in a different memory region than

> +the main clock controller, a separate clock controller has to be defined for

> +handling them.

> +

> +Required Properties:

> +

> +- compatible: should be "samsung,exynos4412-isp-clock".

> +

> +- reg: physical base address of the ISP clock controller and length of memory

> +  mapped region.

> +

> +- #clock-cells: should be 1.

> +

> +- clocks: list of the clock controller input clock identifiers,

> +  from common clock bindings, should point to CLK_ACLK200 and

> +  CLK_ACLK400_MCUISP clocks from the main clock controller.

> +

> +- clock-names: list of the clock controller input clock names,

> +  as described in clock-bindings.txt, should be "aclk200" and

> +  "aclk400_mcuisp".

> +

> +- power-domains: a phandle to ISP power domain node as described by

> +  generic PM domain bindings.

> +

> +Example 3: The clock controllers bindings for Exynos4412 SoCs.

> +

> +	clock: clock-controller@10030000 {

> +		compatible = "samsung,exynos4412-clock";

> +		reg = <0x10030000 0x18000>;

> +		#clock-cells = <1>;

> +	};

> +

> +	isp_clock: clock-controller@10048000 {

> +		compatible = "samsung,exynos4412-isp-clock";

> +		reg = <0x10048000 0x1000>;

> +		#clock-cells = <1>;

> +		power-domains = <&pd_isp>;

> +		clocks = <&clock CLK_ACLK200>, <&clock CLK_ACLK400_MCUISP>;

> +		clock-names = "aclk200", "aclk400_mcuisp";

> +	};

> diff --git a/include/dt-bindings/clock/exynos4.h b/include/dt-bindings/clock/exynos4.h

> index c40111f36d5e..e9f9d400c322 100644

> --- a/include/dt-bindings/clock/exynos4.h

> +++ b/include/dt-bindings/clock/exynos4.h

> @@ -272,4 +272,39 @@

>  /* must be greater than maximal clock id */

>  #define CLK_NR_CLKS		461

>  

> +/* Exynos4x12 ISP clocks */


Shouldn't the clock IDs go with driver changes? I think only the
Documentation/ part should be separate.

Best regards,
Krzysztof


> +#define CLK_ISP_FIMC_ISP		 1

> +#define CLK_ISP_FIMC_DRC		 2

> +#define CLK_ISP_FIMC_FD			 3

> +#define CLK_ISP_FIMC_LITE0		 4

> +#define CLK_ISP_FIMC_LITE1		 5

> +#define CLK_ISP_MCUISP			 6

> +#define CLK_ISP_GICISP			 7

> +#define CLK_ISP_SMMU_ISP		 8

> +#define CLK_ISP_SMMU_DRC		 9

> +#define CLK_ISP_SMMU_FD			10

> +#define CLK_ISP_SMMU_LITE0		11

> +#define CLK_ISP_SMMU_LITE1		12

> +#define CLK_ISP_PPMUISPMX		13

> +#define CLK_ISP_PPMUISPX		14

> +#define CLK_ISP_MCUCTL_ISP		15

> +#define CLK_ISP_MPWM_ISP		16

> +#define CLK_ISP_I2C0_ISP		17

> +#define CLK_ISP_I2C1_ISP		18

> +#define CLK_ISP_MTCADC_ISP		19

> +#define CLK_ISP_PWM_ISP			20

> +#define CLK_ISP_WDT_ISP			21

> +#define CLK_ISP_UART_ISP		22

> +#define CLK_ISP_ASYNCAXIM		23

> +#define CLK_ISP_SMMU_ISPCX		24

> +#define CLK_ISP_SPI0_ISP		25

> +#define CLK_ISP_SPI1_ISP		26

> +

> +#define CLK_ISP_DIV_ISP0		27

> +#define CLK_ISP_DIV_ISP1		28

> +#define CLK_ISP_DIV_MCUISP0		29

> +#define CLK_ISP_DIV_MCUISP1		30

> +

> +#define CLK_NR_ISP_CLKS			31

> +

>  #endif /* _DT_BINDINGS_CLOCK_EXYNOS_4_H */

> -- 

> 2.14.2

> 

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Szyprowski Oct. 12, 2017, 6:47 a.m. | #2
Hi Krzysztof,

On 2017-10-11 19:05, Krzysztof Kozlowski wrote:
> On Wed, Oct 11, 2017 at 11:25:12AM +0200, Marek Szyprowski wrote:

>> Some registers for the Exynos 4412 ISP (Camera subsystem) clocks are

>> located in the ISP power domain. Because those registers are also

>> located in a different memory region than the main clock controller,

>> support for them can be provided by a separate clock controller.

>>

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

>>   .../devicetree/bindings/clock/exynos4-clock.txt    | 43 ++++++++++++++++++++++

>>   include/dt-bindings/clock/exynos4.h                | 35 ++++++++++++++++++

>>   2 files changed, 78 insertions(+)

>>

>> diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt b/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>> index f5a5b19ed3b2..bc61c952cb0b 100644

>> --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>> +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>> @@ -41,3 +41,46 @@ Example 2: UART controller node that consumes the clock generated by the clock

>>   		clocks = <&clock CLK_UART2>, <&clock CLK_SCLK_UART2>;

>>   		clock-names = "uart", "clk_uart_baud0";

>>   	};

>> +

>> +Exynos4412 SoC contains some additional clocks for FIMC-ISP (Camera ISP)

>> +subsystem. Registers for those clocks are located in the ISP power domain.

>> +Because those registers are also located in a different memory region than

>> +the main clock controller, a separate clock controller has to be defined for

>> +handling them.

>> +

>> +Required Properties:

>> +

>> +- compatible: should be "samsung,exynos4412-isp-clock".

>> +

>> +- reg: physical base address of the ISP clock controller and length of memory

>> +  mapped region.

>> +

>> +- #clock-cells: should be 1.

>> +

>> +- clocks: list of the clock controller input clock identifiers,

>> +  from common clock bindings, should point to CLK_ACLK200 and

>> +  CLK_ACLK400_MCUISP clocks from the main clock controller.

>> +

>> +- clock-names: list of the clock controller input clock names,

>> +  as described in clock-bindings.txt, should be "aclk200" and

>> +  "aclk400_mcuisp".

>> +

>> +- power-domains: a phandle to ISP power domain node as described by

>> +  generic PM domain bindings.

>> +

>> +Example 3: The clock controllers bindings for Exynos4412 SoCs.

>> +

>> +	clock: clock-controller@10030000 {

>> +		compatible = "samsung,exynos4412-clock";

>> +		reg = <0x10030000 0x18000>;

>> +		#clock-cells = <1>;

>> +	};

>> +

>> +	isp_clock: clock-controller@10048000 {

>> +		compatible = "samsung,exynos4412-isp-clock";

>> +		reg = <0x10048000 0x1000>;

>> +		#clock-cells = <1>;

>> +		power-domains = <&pd_isp>;

>> +		clocks = <&clock CLK_ACLK200>, <&clock CLK_ACLK400_MCUISP>;

>> +		clock-names = "aclk200", "aclk400_mcuisp";

>> +	};

>> diff --git a/include/dt-bindings/clock/exynos4.h b/include/dt-bindings/clock/exynos4.h

>> index c40111f36d5e..e9f9d400c322 100644

>> --- a/include/dt-bindings/clock/exynos4.h

>> +++ b/include/dt-bindings/clock/exynos4.h

>> @@ -272,4 +272,39 @@

>>   /* must be greater than maximal clock id */

>>   #define CLK_NR_CLKS		461

>>   

>> +/* Exynos4x12 ISP clocks */

> Shouldn't the clock IDs go with driver changes? I think only the

> Documentation/ part should be separate.


Well, Rob asked to move dt bindings and dt include to the separate patch.

I see no value-added by such split, as both dt and driver patches will be
processed together anyway, but I didn't want to have this patchset blocked
by this issue.

 > ...


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Oct. 12, 2017, 7:08 a.m. | #3
On Thu, Oct 12, 2017 at 8:47 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi Krzysztof,

>

>

> On 2017-10-11 19:05, Krzysztof Kozlowski wrote:

>>

>> On Wed, Oct 11, 2017 at 11:25:12AM +0200, Marek Szyprowski wrote:

>>>

>>> Some registers for the Exynos 4412 ISP (Camera subsystem) clocks are

>>> located in the ISP power domain. Because those registers are also

>>> located in a different memory region than the main clock controller,

>>> support for them can be provided by a separate clock controller.

>>>

>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>>> ---

>>>   .../devicetree/bindings/clock/exynos4-clock.txt    | 43

>>> ++++++++++++++++++++++

>>>   include/dt-bindings/clock/exynos4.h                | 35

>>> ++++++++++++++++++

>>>   2 files changed, 78 insertions(+)

>>>

>>> diff --git a/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>>> b/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>>> index f5a5b19ed3b2..bc61c952cb0b 100644

>>> --- a/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>>> +++ b/Documentation/devicetree/bindings/clock/exynos4-clock.txt

>>> @@ -41,3 +41,46 @@ Example 2: UART controller node that consumes the

>>> clock generated by the clock

>>>                 clocks = <&clock CLK_UART2>, <&clock CLK_SCLK_UART2>;

>>>                 clock-names = "uart", "clk_uart_baud0";

>>>         };

>>> +

>>> +Exynos4412 SoC contains some additional clocks for FIMC-ISP (Camera ISP)

>>> +subsystem. Registers for those clocks are located in the ISP power

>>> domain.

>>> +Because those registers are also located in a different memory region

>>> than

>>> +the main clock controller, a separate clock controller has to be defined

>>> for

>>> +handling them.

>>> +

>>> +Required Properties:

>>> +

>>> +- compatible: should be "samsung,exynos4412-isp-clock".

>>> +

>>> +- reg: physical base address of the ISP clock controller and length of

>>> memory

>>> +  mapped region.

>>> +

>>> +- #clock-cells: should be 1.

>>> +

>>> +- clocks: list of the clock controller input clock identifiers,

>>> +  from common clock bindings, should point to CLK_ACLK200 and

>>> +  CLK_ACLK400_MCUISP clocks from the main clock controller.

>>> +

>>> +- clock-names: list of the clock controller input clock names,

>>> +  as described in clock-bindings.txt, should be "aclk200" and

>>> +  "aclk400_mcuisp".

>>> +

>>> +- power-domains: a phandle to ISP power domain node as described by

>>> +  generic PM domain bindings.

>>> +

>>> +Example 3: The clock controllers bindings for Exynos4412 SoCs.

>>> +

>>> +       clock: clock-controller@10030000 {

>>> +               compatible = "samsung,exynos4412-clock";

>>> +               reg = <0x10030000 0x18000>;

>>> +               #clock-cells = <1>;

>>> +       };

>>> +

>>> +       isp_clock: clock-controller@10048000 {

>>> +               compatible = "samsung,exynos4412-isp-clock";

>>> +               reg = <0x10048000 0x1000>;

>>> +               #clock-cells = <1>;

>>> +               power-domains = <&pd_isp>;

>>> +               clocks = <&clock CLK_ACLK200>, <&clock

>>> CLK_ACLK400_MCUISP>;

>>> +               clock-names = "aclk200", "aclk400_mcuisp";

>>> +       };

>>> diff --git a/include/dt-bindings/clock/exynos4.h

>>> b/include/dt-bindings/clock/exynos4.h

>>> index c40111f36d5e..e9f9d400c322 100644

>>> --- a/include/dt-bindings/clock/exynos4.h

>>> +++ b/include/dt-bindings/clock/exynos4.h

>>> @@ -272,4 +272,39 @@

>>>   /* must be greater than maximal clock id */

>>>   #define CLK_NR_CLKS           461

>>>   +/* Exynos4x12 ISP clocks */

>>

>> Shouldn't the clock IDs go with driver changes? I think only the

>> Documentation/ part should be separate.

>

>

> Well, Rob asked to move dt bindings and dt include to the separate patch.

>

> I see no value-added by such split, as both dt and driver patches will be

> processed together anyway, but I didn't want to have this patchset blocked

> by this issue.


I understand... actually I think it does not matter where the clock
IDs would go. From my perspective both approaches look fine so in any
case:
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Oct. 16, 2017, 1:24 p.m. | #4
On 10/11/2017 11:25 AM, Marek Szyprowski wrote:
> Marek Szyprowski (5):

>   clk: samsung: Instantiate Exynos4412 ISP clocks only when available

>   clk: samsung: Add dt bindings for Exynos4412 ISP clock controller

>   clk: samsung: Add a separate driver for Exynos4412 ISP clocks


I have applied these 3 patches, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html