2020-03-10 19:50:31

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 0/5] Add support for FSYS power domain and enable suspend clk for Exynos542x SoC

Series build and tested on linux next-20200306.

This patch series tries to enable FSYS power domain
for USBDRD3, PDMA and USB2.0 devices.
Some new patches is added to enable this feature.

Summary:
# powerdebug -d
...

FSYS:
current_state: on
active_time: 236786 ms
total_idle_time: 1914 ms
Idle States:
State Time
S0 1914
Devices:
/devices/platform/soc/10010000.clock-controller/exynos5-subcmu.6.auto
/devices/platform/soc/12130000.phy
/devices/platform/soc/12100000.phy
/devices/platform/soc/12500000.phy
/devices/platform/soc/soc:amba/121a0000.pdma
/devices/platform/soc/soc:amba/121b0000.pdma
/devices/platform/soc/12110000.usb
/devices/platform/soc/12120000.usb
/devices/platform/soc/soc:usb3-0
/devices/platform/soc/soc:usb3-1

This patch series tries to enable suspend clk using
exynos dwc3 driver, for this I have added new
compatible string "samsung,exynos5420-dwusb3"
so that we could add new suspend clk in addition
to the core clk. exynos dwc3 driver will help
enable/disable these clk.

This series PatchV2.
--Added the clk names for exynos5420 compatible.
--Added missing support for Exyno5410 SoC suspend clock.
--Update the commit message to support suspend clk usages.

---
Long time ago I tried to add suspend clk for dwc3 phy
which was wrong appoch, see below.

[0] https://lore.kernel.org/patchwork/patch/837635/
[1] https://lore.kernel.org/patchwork/patch/837636/

Previous changes V3 (It was send with wrong Patch version)
[2] https://patchwork.kernel.org/cover/11373043/

-Anand

Anand Moon (5):
devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3
clocks support
ARM: dts: exynos: Add missing usbdrd3 suspend clk
ARM: dts: exynos: Add FSYS power domain to Exynos542x USB nodes
usb: dwc3: exynos: Add support for Exynos5422 suspend clk
clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

.../devicetree/bindings/usb/exynos-usb.txt | 5 ++-
arch/arm/boot/dts/exynos5410.dtsi | 8 ++--
arch/arm/boot/dts/exynos5420.dtsi | 24 ++++++++--
arch/arm/boot/dts/exynos54xx.dtsi | 4 +-
drivers/clk/samsung/clk-exynos5420.c | 45 ++++++++++++++-----
drivers/usb/dwc3/dwc3-exynos.c | 9 ++++
6 files changed, 73 insertions(+), 22 deletions(-)

--
2.25.1


2020-03-10 19:50:38

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 4/5] usb: dwc3: exynos: Add support for Exynos5422 suspend clk

Exynos5422 DWC3 module support two clk USBD300 and SCLK_USBD300
so add missing code to enable/disable code and suspend clk, for this
add a new compatible samsung,exynos5420-dwusb3 to help configure
dwc3 code and dwc3 suspend clock. Suspend clock controls the PHY power
change from P0 to P1/P2/P3 during U0 to U1/U2/U3 transition.

Signed-off-by: Anand Moon <[email protected]>
---
drivers/usb/dwc3/dwc3-exynos.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 90bb022737da..48b68b6f0dc8 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -162,6 +162,12 @@ static const struct dwc3_exynos_driverdata exynos5250_drvdata = {
.suspend_clk_idx = -1,
};

+static const struct dwc3_exynos_driverdata exynos5420_drvdata = {
+ .clk_names = { "usbdrd30", "usbdrd30_susp_clk"},
+ .num_clks = 2,
+ .suspend_clk_idx = 1,
+};
+
static const struct dwc3_exynos_driverdata exynos5433_drvdata = {
.clk_names = { "aclk", "susp_clk", "pipe_pclk", "phyclk" },
.num_clks = 4,
@@ -178,6 +184,9 @@ static const struct of_device_id exynos_dwc3_match[] = {
{
.compatible = "samsung,exynos5250-dwusb3",
.data = &exynos5250_drvdata,
+ }, {
+ .compatible = "samsung,exynos5420-dwusb3",
+ .data = &exynos5420_drvdata,
}, {
.compatible = "samsung,exynos5433-dwusb3",
.data = &exynos5433_drvdata,
--
2.25.1

2020-03-10 19:51:19

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 1/5] devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3 clocks support

Add the new compatible string for Exynos5422 DWC3 to support
enable/disable of core and suspend clk by DWC3 driver.
Also updated the clock names for compatible samsung,exynos5420-dwusb3.

Acked-by: Rob Herring <[email protected]>
Signed-off-by: Anand Moon <[email protected]>
---
Documentation/devicetree/bindings/usb/exynos-usb.txt | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
index 6aae1544f240..220f729ac8eb 100644
--- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
+++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
@@ -69,7 +69,9 @@ DWC3
Required properties:
- compatible: should be one of the following -
"samsung,exynos5250-dwusb3": for USB 3.0 DWC3 controller on
- Exynos5250/5420.
+ Exynos5250.
+ "samsung,exynos5420-dwusb3": for USB 3.0 DWC3 controller on
+ Exynos5420.
"samsung,exynos5433-dwusb3": for USB 3.0 DWC3 controller on
Exynos5433.
"samsung,exynos7-dwusb3": for USB 3.0 DWC3 controller on Exynos7.
@@ -82,6 +84,7 @@ Required properties:
Following clock names shall be provided for different
compatibles:
- samsung,exynos5250-dwusb3: "usbdrd30",
+ - samsung,exynos5420-dwusb3: "usbdrd30", "usbdrd30_susp_clk",
- samsung,exynos5433-dwusb3: "aclk", "susp_clk", "pipe_pclk",
"phyclk",
- samsung,exynos7-dwusb3: "usbdrd30", "usbdrd30_susp_clk",
--
2.25.1

2020-03-10 19:51:21

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 3/5] ARM: dts: exynos: Add FSYS power domain to Exynos542x USB nodes

Add a power domain FSYS for USB 3.0 and USB 2.0 and pdma nodes present
on Exynos542x/5800 SoCs.

Signed-off-by: Anand Moon <[email protected]>
---
New patch in this series.
---
arch/arm/boot/dts/exynos5420.dtsi | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index bd505256a223..4046b669b105 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -396,6 +396,13 @@ msc_pd: power-domain@10044120 {
label = "MSC";
};

+ fsys_pd: power-domain@10044140 {
+ compatible = "samsung,exynos4210-pd";
+ reg = <0x10044140 0x20>;
+ #power-domain-cells = <0>;
+ label = "FSYS";
+ };
+
pinctrl_0: pinctrl@13400000 {
compatible = "samsung,exynos5420-pinctrl";
reg = <0x13400000 0x1000>;
@@ -461,6 +468,7 @@ pdma0: pdma@121a0000 {
#dma-cells = <1>;
#dma-channels = <8>;
#dma-requests = <32>;
+ power-domains = <&fsys_pd>;
};

pdma1: pdma@121b0000 {
@@ -472,6 +480,7 @@ pdma1: pdma@121b0000 {
#dma-cells = <1>;
#dma-channels = <8>;
#dma-requests = <32>;
+ power-domains = <&fsys_pd>;
};

mdma0: mdma@10800000 {
@@ -1374,17 +1383,20 @@ &trng {
&usbdrd3_0 {
clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBD300>;
clock-names = "usbdrd30", "usbdrd30_susp_clk";
+ power-domains = <&fsys_pd>;
};

&usbdrd_phy0 {
clocks = <&clock CLK_USBD300>, <&clock CLK_SCLK_USBPHY300>;
clock-names = "phy", "ref";
samsung,pmu-syscon = <&pmu_system_controller>;
+ power-domains = <&fsys_pd>;
};

&usbdrd3_1 {
clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBD301>;
clock-names = "usbdrd30", "usbdrd30_susp_clk";
+ power-domains = <&fsys_pd>;
};

&usbdrd_dwc3_1 {
@@ -1395,16 +1407,19 @@ &usbdrd_phy1 {
clocks = <&clock CLK_USBD301>, <&clock CLK_SCLK_USBPHY301>;
clock-names = "phy", "ref";
samsung,pmu-syscon = <&pmu_system_controller>;
+ power-domains = <&fsys_pd>;
};

&usbhost1 {
clocks = <&clock CLK_USBH20>;
clock-names = "usbhost";
+ power-domains = <&fsys_pd>;
};

&usbhost2 {
clocks = <&clock CLK_USBH20>;
clock-names = "usbhost";
+ power-domains = <&fsys_pd>;
};

&usb2_phy {
@@ -1412,6 +1427,7 @@ &usb2_phy {
clock-names = "phy", "ref";
samsung,sysreg-phandle = <&sysreg_system_controller>;
samsung,pmureg-phandle = <&pmu_system_controller>;
+ power-domains = <&fsys_pd>;
};

&watchdog {
--
2.25.1

2020-03-10 19:51:58

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
hence move FSYS clk setting to sub-CMU block to support power domain
on/off sequences for device nodes.

Signed-off-by: Anand Moon <[email protected]>
---
New patch in the series
---
drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index c9e5a1fb6653..6c4c47dfcdce 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),

- /* USB3.0 */
- DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
- DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
- DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
- DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
-
/* MMC */
DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
@@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {

/* FSYS Block */
GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
- GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
- GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
@@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
- GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
- GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
- GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),

@@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
{ DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
};

+/* USB3.0 */
+static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
+ DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
+ DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
+ DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
+ DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
+};
+
+static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
+ GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
+ GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
+ GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
+ GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
+ GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
+};
+
+static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
+ { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */
+ { SRC_TOP3, 0, BIT(24) }, /* SW_MUX_PCLK_200_FSYS_SEL */
+ { SRC_TOP3, 0, BIT(28) }, /* SW_MUX_ACLK_200_FSYS_SEL */
+};
+
static const struct samsung_gate_clock exynos5x_g3d_gate_clks[] __initconst = {
GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9,
CLK_SET_RATE_PARENT, 0),
@@ -1376,12 +1387,23 @@ static const struct exynos5_subcmu_info exynos5800_mau_subcmu = {
.pd_name = "MAU",
};

+static const struct exynos5_subcmu_info exynos5x_fsys_subcmu = {
+ .div_clks = exynos5x_fsys_div_clks,
+ .nr_div_clks = ARRAY_SIZE(exynos5x_fsys_div_clks),
+ .gate_clks = exynos5x_fsys_gate_clks,
+ .nr_gate_clks = ARRAY_SIZE(exynos5x_fsys_gate_clks),
+ .suspend_regs = exynos5x_fsys_suspend_regs,
+ .nr_suspend_regs = ARRAY_SIZE(exynos5x_fsys_suspend_regs),
+ .pd_name = "FSYS",
+};
+
static const struct exynos5_subcmu_info *exynos5x_subcmus[] = {
&exynos5x_disp_subcmu,
&exynos5x_gsc_subcmu,
&exynos5x_g3d_subcmu,
&exynos5x_mfc_subcmu,
&exynos5x_mscl_subcmu,
+ &exynos5x_fsys_subcmu,
};

static const struct exynos5_subcmu_info *exynos5800_subcmus[] = {
@@ -1391,6 +1413,7 @@ static const struct exynos5_subcmu_info *exynos5800_subcmus[] = {
&exynos5x_mfc_subcmu,
&exynos5x_mscl_subcmu,
&exynos5800_mau_subcmu,
+ &exynos5x_fsys_subcmu,
};

static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __initconst = {
--
2.25.1

2020-03-11 14:43:51

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

On Tue, Mar 10, 2020 at 07:48:54PM +0000, Anand Moon wrote:
> FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
> hence move FSYS clk setting to sub-CMU block to support power domain
> on/off sequences for device nodes.
>
> Signed-off-by: Anand Moon <[email protected]>
> ---
> New patch in the series
> ---
> drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
> 1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index c9e5a1fb6653..6c4c47dfcdce 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
> DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
> DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),
>
> - /* USB3.0 */
> - DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> - DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> - DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> - DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),

According to clock diagram these are still in CMU TOP, not FSYS.

> -
> /* MMC */
> DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
> DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
> @@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
/>
> /* FSYS Block */
> GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
> - GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> - GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
> GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
> GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
> @@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
> GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
> GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
> - GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> - GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> - GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
> SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),
>
> @@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
> { DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
> };
>
> +/* USB3.0 */
> +static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
> + DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> + DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> + DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> + DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> +};
> +
> +static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
> + GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> + GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> + GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> + GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> + GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> +};
> +
> +static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
> + { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */

This looks wrong. GATE_IP_FSYS has fields also for FSYS2 clocks which
you are not suspending. They do not belong to this CMU.

Don't you need to save also parts of GATE_BUS_FSYS0?

> + { SRC_TOP3, 0, BIT(24) }, /* SW_MUX_PCLK_200_FSYS_SEL */
> + { SRC_TOP3, 0, BIT(28) }, /* SW_MUX_ACLK_200_FSYS_SEL */

Name of clocks from the driver please, not from datasheet. Look at other
examples.

Best regards,
Krzysztof


> +};
> +
> static const struct samsung_gate_clock exynos5x_g3d_gate_clks[] __initconst = {
> GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9,
> CLK_SET_RATE_PARENT, 0),
> @@ -1376,12 +1387,23 @@ static const struct exynos5_subcmu_info exynos5800_mau_subcmu = {
> .pd_name = "MAU",
> };
>
> +static const struct exynos5_subcmu_info exynos5x_fsys_subcmu = {
> + .div_clks = exynos5x_fsys_div_clks,
> + .nr_div_clks = ARRAY_SIZE(exynos5x_fsys_div_clks),
> + .gate_clks = exynos5x_fsys_gate_clks,
> + .nr_gate_clks = ARRAY_SIZE(exynos5x_fsys_gate_clks),
> + .suspend_regs = exynos5x_fsys_suspend_regs,
> + .nr_suspend_regs = ARRAY_SIZE(exynos5x_fsys_suspend_regs),
> + .pd_name = "FSYS",
> +};
> +
> static const struct exynos5_subcmu_info *exynos5x_subcmus[] = {
> &exynos5x_disp_subcmu,
> &exynos5x_gsc_subcmu,
> &exynos5x_g3d_subcmu,
> &exynos5x_mfc_subcmu,
> &exynos5x_mscl_subcmu,
> + &exynos5x_fsys_subcmu,
> };
>
> static const struct exynos5_subcmu_info *exynos5800_subcmus[] = {
> @@ -1391,6 +1413,7 @@ static const struct exynos5_subcmu_info *exynos5800_subcmus[] = {
> &exynos5x_mfc_subcmu,
> &exynos5x_mscl_subcmu,
> &exynos5800_mau_subcmu,
> + &exynos5x_fsys_subcmu,
> };
>
> static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __initconst = {
> --
> 2.25.1
>

2020-03-12 10:35:56

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

Hi Krzysztof,

Thanks for your review comments.

On Wed, 11 Mar 2020 at 20:12, Krzysztof Kozlowski <[email protected]> wrote:
>
> On Tue, Mar 10, 2020 at 07:48:54PM +0000, Anand Moon wrote:
> > FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
> > hence move FSYS clk setting to sub-CMU block to support power domain
> > on/off sequences for device nodes.
> >
> > Signed-off-by: Anand Moon <[email protected]>
> > ---
> > New patch in the series
> > ---
> > drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
> > 1 file changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> > index c9e5a1fb6653..6c4c47dfcdce 100644
> > --- a/drivers/clk/samsung/clk-exynos5420.c
> > +++ b/drivers/clk/samsung/clk-exynos5420.c
> > @@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
> > DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
> > DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),
> >
> > - /* USB3.0 */
> > - DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > - DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > - DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > - DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
>
> According to clock diagram these are still in CMU TOP, not FSYS.
>
> > -
> > /* MMC */
> > DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
> > DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
> > @@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> />
> > /* FSYS Block */
> > GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
> > - GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > - GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
> > GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
> > GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
> > @@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
> > GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
> > GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
> > - GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > - GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > - GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
> > SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),
> >
> > @@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
> > { DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
> > };
> >
> > +/* USB3.0 */
> > +static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
> > + DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > + DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > + DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > + DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > +};
> > +
> > +static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
> > + GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > + GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > + GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > + GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > + GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > +};
> > +
> > +static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
> > + { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */
>
> This looks wrong. GATE_IP_FSYS has fields also for FSYS2 clocks which
> you are not suspending. They do not belong to this CMU.
>

Ok. I change the from GATE_IP_FSYS to GATE_BUS_FSYS0 in the above
exynos5x_fsys_gate_clks to make this consistent to used GATE_BUS_FSYS0 for CMU,
with this change it works as per previously.

> Don't you need to save also parts of GATE_BUS_FSYS0?

GATE_BUS_FSYS0 and GATE_IP_FSYS are already part of list
of control register which are saved and restored during suspend and resume
so no point in adding this here, I should drop the GATE_IP_FSYS reg
dump over here.

>
> > + { SRC_TOP3, 0, BIT(24) }, /* SW_MUX_PCLK_200_FSYS_SEL */
> > + { SRC_TOP3, 0, BIT(28) }, /* SW_MUX_ACLK_200_FSYS_SEL */
>
> Name of clocks from the driver please, not from datasheet. Look at other
> examples.
>

Ok I will update this as per the examples.

> Best regards,
> Krzysztof
>
>

-Anand

> > +};
> > +
> > static const struct samsung_gate_clock exynos5x_g3d_gate_clks[] __initconst = {
> > GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9,
> > CLK_SET_RATE_PARENT, 0),
> > @@ -1376,12 +1387,23 @@ static const struct exynos5_subcmu_info exynos5800_mau_subcmu = {
> > .pd_name = "MAU",
> > };
> >
> > +static const struct exynos5_subcmu_info exynos5x_fsys_subcmu = {
> > + .div_clks = exynos5x_fsys_div_clks,
> > + .nr_div_clks = ARRAY_SIZE(exynos5x_fsys_div_clks),
> > + .gate_clks = exynos5x_fsys_gate_clks,
> > + .nr_gate_clks = ARRAY_SIZE(exynos5x_fsys_gate_clks),
> > + .suspend_regs = exynos5x_fsys_suspend_regs,
> > + .nr_suspend_regs = ARRAY_SIZE(exynos5x_fsys_suspend_regs),
> > + .pd_name = "FSYS",
> > +};
> > +
> > static const struct exynos5_subcmu_info *exynos5x_subcmus[] = {
> > &exynos5x_disp_subcmu,
> > &exynos5x_gsc_subcmu,
> > &exynos5x_g3d_subcmu,
> > &exynos5x_mfc_subcmu,
> > &exynos5x_mscl_subcmu,
> > + &exynos5x_fsys_subcmu,
> > };
> >
> > static const struct exynos5_subcmu_info *exynos5800_subcmus[] = {
> > @@ -1391,6 +1413,7 @@ static const struct exynos5_subcmu_info *exynos5800_subcmus[] = {
> > &exynos5x_mfc_subcmu,
> > &exynos5x_mscl_subcmu,
> > &exynos5800_mau_subcmu,
> > + &exynos5x_fsys_subcmu,
> > };
> >
> > static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __initconst = {
> > --
> > 2.25.1
> >

2020-03-12 11:37:00

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

On Thu, Mar 12, 2020 at 04:04:57PM +0530, Anand Moon wrote:
> Hi Krzysztof,
>
> Thanks for your review comments.
>
> On Wed, 11 Mar 2020 at 20:12, Krzysztof Kozlowski <[email protected]> wrote:
> >
> > On Tue, Mar 10, 2020 at 07:48:54PM +0000, Anand Moon wrote:
> > > FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
> > > hence move FSYS clk setting to sub-CMU block to support power domain
> > > on/off sequences for device nodes.
> > >
> > > Signed-off-by: Anand Moon <[email protected]>
> > > ---
> > > New patch in the series
> > > ---
> > > drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
> > > 1 file changed, 34 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> > > index c9e5a1fb6653..6c4c47dfcdce 100644
> > > --- a/drivers/clk/samsung/clk-exynos5420.c
> > > +++ b/drivers/clk/samsung/clk-exynos5420.c
> > > @@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
> > > DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
> > > DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),
> > >
> > > - /* USB3.0 */
> > > - DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > - DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > - DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > - DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> >
> > According to clock diagram these are still in CMU TOP, not FSYS.
> >
> > > -
> > > /* MMC */
> > > DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
> > > DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
> > > @@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > />
> > > /* FSYS Block */
> > > GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
> > > - GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > - GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
> > > GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
> > > GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
> > > @@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
> > > GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
> > > GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
> > > - GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > - GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > - GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
> > > SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),
> > >
> > > @@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
> > > { DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
> > > };
> > >
> > > +/* USB3.0 */
> > > +static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
> > > + DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > + DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > + DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > + DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > > +};
> > > +
> > > +static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
> > > + GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > + GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > + GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > + GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > + GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > +};
> > > +
> > > +static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
> > > + { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */
> >
> > This looks wrong. GATE_IP_FSYS has fields also for FSYS2 clocks which
> > you are not suspending. They do not belong to this CMU.
> >
>
> Ok. I change the from GATE_IP_FSYS to GATE_BUS_FSYS0 in the above
> exynos5x_fsys_gate_clks to make this consistent to used GATE_BUS_FSYS0 for CMU,
> with this change it works as per previously.

Wait, you should set here proper registers with proper mask.
>
> > Don't you need to save also parts of GATE_BUS_FSYS0?
>
> GATE_BUS_FSYS0 and GATE_IP_FSYS are already part of list
> of control register which are saved and restored during suspend and resume
> so no point in adding this here, I should drop the GATE_IP_FSYS reg
> dump over here.

Since registers are used in separate sub CMU devices, each should
save/restore its part.

Best regards,
Krzysztof

2020-03-12 12:55:20

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

Hi Krzysztof,

On Thu, 12 Mar 2020 at 17:06, Krzysztof Kozlowski <[email protected]> wrote:
>
> On Thu, Mar 12, 2020 at 04:04:57PM +0530, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > Thanks for your review comments.
> >
> > On Wed, 11 Mar 2020 at 20:12, Krzysztof Kozlowski <[email protected]> wrote:
> > >
> > > On Tue, Mar 10, 2020 at 07:48:54PM +0000, Anand Moon wrote:
> > > > FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
> > > > hence move FSYS clk setting to sub-CMU block to support power domain
> > > > on/off sequences for device nodes.
> > > >
> > > > Signed-off-by: Anand Moon <[email protected]>
> > > > ---
> > > > New patch in the series
> > > > ---
> > > > drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
> > > > 1 file changed, 34 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> > > > index c9e5a1fb6653..6c4c47dfcdce 100644
> > > > --- a/drivers/clk/samsung/clk-exynos5420.c
> > > > +++ b/drivers/clk/samsung/clk-exynos5420.c
> > > > @@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
> > > > DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
> > > > DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),
> > > >
> > > > - /* USB3.0 */
> > > > - DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > > - DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > > - DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > > - DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > >
> > > According to clock diagram these are still in CMU TOP, not FSYS.
> > >
> > > > -
> > > > /* MMC */
> > > > DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
> > > > DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
> > > > @@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > />
> > > > /* FSYS Block */
> > > > GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
> > > > - GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > > - GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > > GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
> > > > GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
> > > > GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
> > > > @@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > > GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
> > > > GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
> > > > GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
> > > > - GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > > - GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > > - GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > > GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
> > > > SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),
> > > >
> > > > @@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
> > > > { DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
> > > > };
> > > >
> > > > +/* USB3.0 */
> > > > +static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
> > > > + DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > > + DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > > + DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > > + DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > > > +};
> > > > +
> > > > +static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
> > > > + GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > > + GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > > + GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > > + GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > > + GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > > +};
> > > > +
> > > > +static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
> > > > + { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */
> > >
> > > This looks wrong. GATE_IP_FSYS has fields also for FSYS2 clocks which
> > > you are not suspending. They do not belong to this CMU.
> > >
> >
> > Ok. I change the from GATE_IP_FSYS to GATE_BUS_FSYS0 in the above
> > exynos5x_fsys_gate_clks to make this consistent to used GATE_BUS_FSYS0 for CMU,
> > with this change it works as per previously.
>
> Wait, you should set here proper registers with proper mask.

Yes I will set the proper mask for each as per the Exynos 5422 User Manual.

Here is what I feel
CLK_GATE_BUS_FSYS0 controls the PHY clock
CLK_GATE_IP_FSYS controls the IP clock.

So both these field should be part of this FSYS CMU.

> >
> > > Don't you need to save also parts of GATE_BUS_FSYS0?
> >
> > GATE_BUS_FSYS0 and GATE_IP_FSYS are already part of list
> > of control register which are saved and restored during suspend and resume
> > so no point in adding this here, I should drop the GATE_IP_FSYS reg
> > dump over here.
>
> Since registers are used in separate sub CMU devices, each should
> save/restore its part.

Ok I will add both GATE_BUS_FSYS0 and GATE_IP_FSYS
reset value over here.

>
> Best regards,
> Krzysztof
>

-Anand

2020-03-12 14:09:50

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

Hi Krzysztof,

On Thu, 12 Mar 2020 at 18:24, Anand Moon <[email protected]> wrote:
>
> Hi Krzysztof,
>
> On Thu, 12 Mar 2020 at 17:06, Krzysztof Kozlowski <[email protected]> wrote:
> >
> > On Thu, Mar 12, 2020 at 04:04:57PM +0530, Anand Moon wrote:
> > > Hi Krzysztof,
> > >
> > > Thanks for your review comments.
> > >
> > > On Wed, 11 Mar 2020 at 20:12, Krzysztof Kozlowski <[email protected]> wrote:
> > > >
> > > > On Tue, Mar 10, 2020 at 07:48:54PM +0000, Anand Moon wrote:
> > > > > FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
> > > > > hence move FSYS clk setting to sub-CMU block to support power domain
> > > > > on/off sequences for device nodes.
> > > > >
> > > > > Signed-off-by: Anand Moon <[email protected]>
> > > > > ---
> > > > > New patch in the series
> > > > > ---
> > > > > drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
> > > > > 1 file changed, 34 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> > > > > index c9e5a1fb6653..6c4c47dfcdce 100644
> > > > > --- a/drivers/clk/samsung/clk-exynos5420.c
> > > > > +++ b/drivers/clk/samsung/clk-exynos5420.c
> > > > > @@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
> > > > > DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
> > > > > DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),
> > > > >
> > > > > - /* USB3.0 */
> > > > > - DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > > > - DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > > > - DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > > > - DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > > >
> > > > According to clock diagram these are still in CMU TOP, not FSYS.
> > > >
> > > > > -
> > > > > /* MMC */
> > > > > DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
> > > > > DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
> > > > > @@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > > />
> > > > > /* FSYS Block */
> > > > > GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
> > > > > - GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > > > - GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > > > GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
> > > > > GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
> > > > > GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
> > > > > @@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > > > GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
> > > > > GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
> > > > > GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
> > > > > - GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > > > - GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > > > - GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > > > GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
> > > > > SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),
> > > > >
> > > > > @@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
> > > > > { DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
> > > > > };
> > > > >
> > > > > +/* USB3.0 */
> > > > > +static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
> > > > > + DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > > > + DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > > > + DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > > > + DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > > > > +};
> > > > > +
> > > > > +static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
> > > > > + GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > > > + GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > > > + GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > > > + GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > > > + GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > > > +};
> > > > > +
> > > > > +static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
> > > > > + { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */
> > > >
> > > > This looks wrong. GATE_IP_FSYS has fields also for FSYS2 clocks which
> > > > you are not suspending. They do not belong to this CMU.
> > > >
> > >
> > > Ok. I change the from GATE_IP_FSYS to GATE_BUS_FSYS0 in the above
> > > exynos5x_fsys_gate_clks to make this consistent to used GATE_BUS_FSYS0 for CMU,
> > > with this change it works as per previously.
> >
> > Wait, you should set here proper registers with proper mask.
>
> Yes I will set the proper mask for each as per the Exynos 5422 User Manual.
>
> Here is what I feel
> CLK_GATE_BUS_FSYS0 controls the PHY clock
> CLK_GATE_IP_FSYS controls the IP clock.
>

Sorry I cannot register both CLK_GATE_BUS_FSYS0 and CLK_GATE_IP_FSYS
to aclk200_fsys, so I got some error like below.

[ 0.922693] samsung_clk_register_gate: failed to register clock usbh20
[ 0.922857] samsung_clk_register_gate: failed to register clock usbd300
[ 0.923000] samsung_clk_register_gate: failed to register clock usbd301

> So both these field should be part of this FSYS CMU.
>
> > >
> > > > Don't you need to save also parts of GATE_BUS_FSYS0?
> > >
> > > GATE_BUS_FSYS0 and GATE_IP_FSYS are already part of list
> > > of control register which are saved and restored during suspend and resume
> > > so no point in adding this here, I should drop the GATE_IP_FSYS reg
> > > dump over here.
> >
> > Since registers are used in separate sub CMU devices, each should
> > save/restore its part.
>
> Ok I will add both GATE_BUS_FSYS0 and GATE_IP_FSYS
> reset value over here.
>

So only changes to this patch is to set the above correctly.

-Anand

2020-03-15 03:09:48

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCHv3 5/5] clk: samsung: exynos542x: Move FSYS subsystem clocks to its sub-CMU

On Thu, Mar 12, 2020 at 07:38:30PM +0530, Anand Moon wrote:
> Hi Krzysztof,
>
> On Thu, 12 Mar 2020 at 18:24, Anand Moon <[email protected]> wrote:
> >
> > Hi Krzysztof,
> >
> > On Thu, 12 Mar 2020 at 17:06, Krzysztof Kozlowski <[email protected]> wrote:
> > >
> > > On Thu, Mar 12, 2020 at 04:04:57PM +0530, Anand Moon wrote:
> > > > Hi Krzysztof,
> > > >
> > > > Thanks for your review comments.
> > > >
> > > > On Wed, 11 Mar 2020 at 20:12, Krzysztof Kozlowski <[email protected]> wrote:
> > > > >
> > > > > On Tue, Mar 10, 2020 at 07:48:54PM +0000, Anand Moon wrote:
> > > > > > FSYS power domain support usbdrd3, pdma and usb2 power gaiting,
> > > > > > hence move FSYS clk setting to sub-CMU block to support power domain
> > > > > > on/off sequences for device nodes.
> > > > > >
> > > > > > Signed-off-by: Anand Moon <[email protected]>
> > > > > > ---
> > > > > > New patch in the series
> > > > > > ---
> > > > > > drivers/clk/samsung/clk-exynos5420.c | 45 +++++++++++++++++++++-------
> > > > > > 1 file changed, 34 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> > > > > > index c9e5a1fb6653..6c4c47dfcdce 100644
> > > > > > --- a/drivers/clk/samsung/clk-exynos5420.c
> > > > > > +++ b/drivers/clk/samsung/clk-exynos5420.c
> > > > > > @@ -859,12 +859,6 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
> > > > > > DIV(0, "dout_maudio0", "mout_maudio0", DIV_MAU, 20, 4),
> > > > > > DIV(0, "dout_maupcm0", "dout_maudio0", DIV_MAU, 24, 8),
> > > > > >
> > > > > > - /* USB3.0 */
> > > > > > - DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > > > > - DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > > > > - DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > > > > - DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > > > >
> > > > > According to clock diagram these are still in CMU TOP, not FSYS.
> > > > >
> > > > > > -
> > > > > > /* MMC */
> > > > > > DIV(0, "dout_mmc0", "mout_mmc0", DIV_FSYS1, 0, 10),
> > > > > > DIV(0, "dout_mmc1", "mout_mmc1", DIV_FSYS1, 10, 10),
> > > > > > @@ -1031,8 +1025,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > > > />
> > > > > > /* FSYS Block */
> > > > > > GATE(CLK_TSI, "tsi", "aclk200_fsys", GATE_BUS_FSYS0, 0, 0, 0),
> > > > > > - GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > > > > - GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > > > > GATE(CLK_UFS, "ufs", "aclk200_fsys2", GATE_BUS_FSYS0, 3, 0, 0),
> > > > > > GATE(CLK_RTIC, "rtic", "aclk200_fsys", GATE_IP_FSYS, 9, 0, 0),
> > > > > > GATE(CLK_MMC0, "mmc0", "aclk200_fsys2", GATE_IP_FSYS, 12, 0, 0),
> > > > > > @@ -1040,9 +1032,6 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
> > > > > > GATE(CLK_MMC2, "mmc2", "aclk200_fsys2", GATE_IP_FSYS, 14, 0, 0),
> > > > > > GATE(CLK_SROMC, "sromc", "aclk200_fsys2",
> > > > > > GATE_IP_FSYS, 17, CLK_IGNORE_UNUSED, 0),
> > > > > > - GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > > > > - GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > > > > - GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > > > > GATE(CLK_SCLK_UNIPRO, "sclk_unipro", "dout_unipro",
> > > > > > SRC_MASK_FSYS, 24, CLK_SET_RATE_PARENT, 0),
> > > > > >
> > > > > > @@ -1258,6 +1247,28 @@ static struct exynos5_subcmu_reg_dump exynos5x_gsc_suspend_regs[] = {
> > > > > > { DIV2_RATIO0, 0, 0x30 }, /* DIV dout_gscl_blk_300 */
> > > > > > };
> > > > > >
> > > > > > +/* USB3.0 */
> > > > > > +static const struct samsung_div_clock exynos5x_fsys_div_clks[] __initconst = {
> > > > > > + DIV(0, "dout_usbphy301", "mout_usbd301", DIV_FSYS0, 12, 4),
> > > > > > + DIV(0, "dout_usbphy300", "mout_usbd300", DIV_FSYS0, 16, 4),
> > > > > > + DIV(0, "dout_usbd301", "mout_usbd301", DIV_FSYS0, 20, 4),
> > > > > > + DIV(0, "dout_usbd300", "mout_usbd300", DIV_FSYS0, 24, 4),
> > > > > > +};
> > > > > > +
> > > > > > +static const struct samsung_gate_clock exynos5x_fsys_gate_clks[] __initconst = {
> > > > > > + GATE(CLK_PDMA0, "pdma0", "aclk200_fsys", GATE_BUS_FSYS0, 1, 0, 0),
> > > > > > + GATE(CLK_PDMA1, "pdma1", "aclk200_fsys", GATE_BUS_FSYS0, 2, 0, 0),
> > > > > > + GATE(CLK_USBH20, "usbh20", "aclk200_fsys", GATE_IP_FSYS, 18, 0, 0),
> > > > > > + GATE(CLK_USBD300, "usbd300", "aclk200_fsys", GATE_IP_FSYS, 19, 0, 0),
> > > > > > + GATE(CLK_USBD301, "usbd301", "aclk200_fsys", GATE_IP_FSYS, 20, 0, 0),
> > > > > > +};
> > > > > > +
> > > > > > +static struct exynos5_subcmu_reg_dump exynos5x_fsys_suspend_regs[] = {
> > > > > > + { GATE_IP_FSYS, 0xffffffff, 0xffffffff }, /* FSYS gates */
> > > > >
> > > > > This looks wrong. GATE_IP_FSYS has fields also for FSYS2 clocks which
> > > > > you are not suspending. They do not belong to this CMU.
> > > > >
> > > >
> > > > Ok. I change the from GATE_IP_FSYS to GATE_BUS_FSYS0 in the above
> > > > exynos5x_fsys_gate_clks to make this consistent to used GATE_BUS_FSYS0 for CMU,
> > > > with this change it works as per previously.
> > >
> > > Wait, you should set here proper registers with proper mask.
> >
> > Yes I will set the proper mask for each as per the Exynos 5422 User Manual.
> >
> > Here is what I feel
> > CLK_GATE_BUS_FSYS0 controls the PHY clock
> > CLK_GATE_IP_FSYS controls the IP clock.
> >
>
> Sorry I cannot register both CLK_GATE_BUS_FSYS0 and CLK_GATE_IP_FSYS
> to aclk200_fsys, so I got some error like below.
>
> [ 0.922693] samsung_clk_register_gate: failed to register clock usbh20
> [ 0.922857] samsung_clk_register_gate: failed to register clock usbd300
> [ 0.923000] samsung_clk_register_gate: failed to register clock usbd301
>
> > So both these field should be part of this FSYS CMU.

I am not sure if I understand your problem. I mentioned that you need
to put proper registers with proper masks into the
exynos5_subcmu_reg_dump. I don't know what have you changed to produce
such error logs.

Best regards,
Krzysztof

2020-03-15 09:08:57

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCHv3 1/5] devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3 clocks support


Hi,

Anand Moon <[email protected]> writes:

> Add the new compatible string for Exynos5422 DWC3 to support
> enable/disable of core and suspend clk by DWC3 driver.
> Also updated the clock names for compatible samsung,exynos5420-dwusb3.
>
> Acked-by: Rob Herring <[email protected]>
> Signed-off-by: Anand Moon <[email protected]>

What is the dependency here?

checking file Documentation/devicetree/bindings/usb/exynos-usb.txt
Hunk #2 FAILED at 84.
1 out of 2 hunks FAILED

Applying on top of v5.6-rc5

--
balbi


Attachments:
signature.asc (847.00 B)

2020-03-15 09:25:57

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCHv3 1/5] devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3 clocks support

Hi Felipe,

On Sun, 15 Mar 2020 at 14:37, Felipe Balbi <[email protected]> wrote:
>
>
> Hi,
>
> Anand Moon <[email protected]> writes:
>
> > Add the new compatible string for Exynos5422 DWC3 to support
> > enable/disable of core and suspend clk by DWC3 driver.
> > Also updated the clock names for compatible samsung,exynos5420-dwusb3.
> >
> > Acked-by: Rob Herring <[email protected]>
> > Signed-off-by: Anand Moon <[email protected]>
>
> What is the dependency here?
>
> checking file Documentation/devicetree/bindings/usb/exynos-usb.txt
> Hunk #2 FAILED at 84.
> 1 out of 2 hunks FAILED
>
> Applying on top of v5.6-rc5
>
> --
> balbi

These patch were made on top linux next-20200306,
And with new updates in the clk driver configuration.
I will update these patchs later, plz drop these changes for now.

-Anand

2020-03-17 08:43:13

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCHv3 1/5] devicetree: bindings: exynos: Add new compatible for Exynos5420 dwc3 clocks support

On Sun, 15 Mar 2020 at 10:08, Felipe Balbi <[email protected]> wrote:
>
>
> Hi,
>
> Anand Moon <[email protected]> writes:
>
> > Add the new compatible string for Exynos5422 DWC3 to support
> > enable/disable of core and suspend clk by DWC3 driver.
> > Also updated the clock names for compatible samsung,exynos5420-dwusb3.
> >
> > Acked-by: Rob Herring <[email protected]>
> > Signed-off-by: Anand Moon <[email protected]>
>
> What is the dependency here?

Felipe,

This patchset should not be applied. As of now, it is not needed and
not justified.

Best regards,
Krzysztof