[PATCH 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[PATCH 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[Enric Balletbo i Serra]

Dear all,

These patches is an attempt to improve a little bit the rk3399_dmc
driver and it's documentation in order to have all in a better shape for
a future work I am doing. My final intention is add ddrfreq support for
rockchip drm driver, but the patches for this are still
work-in-progress. So let's start with this first patchset that is
basically some fixes/improvements for the rk3399_dmc driver.

Best regards,

Enric Balletbo i Serra (3):
  dt-bindings: clock: add DDR3 standard speed bins.
  devfreq: rk3399_dmc: remove wait for dcf irq event.
  dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not
    required.

Lin Huang (2):
  devfreq: rk3399_dmc: do not print error when get supply and clk defer.
  devfreq: rk3399_dmc: register devfreq notification to dmc driver.

Nick Milner (1):
  dt-bindings: devfreq: rk3399_dmc: improve binding documentation.

 .../bindings/devfreq/rk3399_dmc.txt           | 204 +++++++++---------
 drivers/devfreq/rk3399_dmc.c                  | 106 +--------
 drivers/soc/rockchip/pm_domains.c             |  31 +++
 include/dt-bindings/clock/ddr.h               |  34 +++
 include/soc/rockchip/rk3399_dmc.h             |  63 ++++++
 5 files changed, 238 insertions(+), 200 deletions(-)
 create mode 100644 include/dt-bindings/clock/ddr.h
 create mode 100644 include/soc/rockchip/rk3399_dmc.h

-- 
2.17.0

[PATCH 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[Enric Balletbo i Serra]

Dear all,

These patches is an attempt to improve a little bit the rk3399_dmc
driver and it's documentation in order to have all in a better shape for
a future work I am doing. My final intention is add ddrfreq support for
rockchip drm driver, but the patches for this are still
work-in-progress. So let's start with this first patchset that is
basically some fixes/improvements for the rk3399_dmc driver.

Best regards,

Enric Balletbo i Serra (3):
  dt-bindings: clock: add DDR3 standard speed bins.
  devfreq: rk3399_dmc: remove wait for dcf irq event.
  dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not
    required.

Lin Huang (2):
  devfreq: rk3399_dmc: do not print error when get supply and clk defer.
  devfreq: rk3399_dmc: register devfreq notification to dmc driver.

Nick Milner (1):
  dt-bindings: devfreq: rk3399_dmc: improve binding documentation.

 .../bindings/devfreq/rk3399_dmc.txt           | 204 +++++++++---------
 drivers/devfreq/rk3399_dmc.c                  | 106 +--------
 drivers/soc/rockchip/pm_domains.c             |  31 +++
 include/dt-bindings/clock/ddr.h               |  34 +++
 include/soc/rockchip/rk3399_dmc.h             |  63 ++++++
 5 files changed, 238 insertions(+), 200 deletions(-)
 create mode 100644 include/dt-bindings/clock/ddr.h
 create mode 100644 include/soc/rockchip/rk3399_dmc.h

-- 
2.17.0

[PATCH 1/6] dt-bindings: devfreq: rk3399_dmc: improve binding documentation.

[Enric Balletbo i Serra]

From: Nick Milner <nick.milner@collabora.com>

There are several typos, references to non existent files, grammar and
punctuation mistakes in the rk3399_dmc.txt binding. This patch tries
to improve the binding documentation and fix these mistakes.

Signed-off-by: Nick Milner <nick.milner@collabora.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 .../bindings/devfreq/rk3399_dmc.txt           | 207 +++++++++---------
 1 file changed, 105 insertions(+), 102 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
index d6d2833482c9..d83ef821d282 100644
--- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
+++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
@@ -1,155 +1,158 @@
-* Rockchip rk3399 DMC(Dynamic Memory Controller) device
+* Rockchip rk3399 DMC (Dynamic Memory Controller) device
 
 Required properties:
 - compatible:		 Must be "rockchip,rk3399-dmc".
 - devfreq-events:	 Node to get DDR loading, Refer to
-			 Documentation/devicetree/bindings/devfreq/
+			 Documentation/devicetree/bindings/devfreq/event/
 			 rockchip-dfi.txt
-- interrupts:		 The interrupt number to the CPU. The interrupt
-			 specifier format depends on the interrupt controller.
-			 It should be DCF interrupts, when DDR dvfs finish,
-			 it will happen.
+- interrupts:		 The CPU interrupt number. The interrupt specifier
+			 format depends on the interrupt controller.
+			 It should be a DCF interrupt. When DDR DVFS finishes
+			 a DCF interrupt is triggered.
 - clocks:		 Phandles for clock specified in "clock-names" property
 - clock-names :		 The name of clock used by the DFI, must be
 			 "pclk_ddr_mon";
-- operating-points-v2:	 Refer to Documentation/devicetree/bindings/power/opp.txt
+- operating-points-v2:	 Refer to Documentation/devicetree/bindings/opp/opp.txt
 			 for details.
 - center-supply:	 DMC supply node.
 - status:		 Marks the node enabled/disabled.
 
-Following properties are ddr timing:
+Following properties relate to DDR timing:
 
 - rockchip,dram_speed_bin :	  Value reference include/dt-bindings/clock/ddr.h,
-				  it select ddr3 cl-trp-trcd type, default value
-				  "DDR3_DEFAULT".it must selected according to
-				  "Speed Bin" in ddr3 datasheet, DO NOT use
-				  smaller "Speed Bin" than ddr3 exactly is.
-
-- rockchip,pd_idle :		  Config the PD_IDLE value, defined the power-down
-				  idle period, memories are places into power-down
-				  mode if bus is idle for PD_IDLE DFI clocks.
-
-- rockchip,sr_idle :		  Configure the SR_IDLE value, defined the
-				  selfrefresh idle period, memories are places
-				  into self-refresh mode if bus is idle for
-				  SR_IDLE*1024 DFI clocks (DFI clocks freq is
-				  half of dram's clocks), defaule value is "0".
-
-- rockchip,sr_mc_gate_idle :	  Defined the self-refresh with memory and
-				  controller clock gating idle period, memories
-				  are places into self-refresh mode and memory
-				  controller clock arg gating if bus is idle for
-				  sr_mc_gate_idle*1024 DFI clocks.
-
-- rockchip,srpd_lite_idle :	  Defined the self-refresh power down idle
-				  period, memories are places into self-refresh
-				  power down mode if bus is idle for
-				  srpd_lite_idle*1024 DFI clocks. This parameter
-				  is for LPDDR4 only.
-
-- rockchip,standby_idle :	  Defined the standby idle period, memories are
-				  places into self-refresh than controller, pi,
-				  phy and dram clock will gating if bus is idle
-				  for standby_idle * DFI clocks.
-
-- rockchip,dram_dll_disb_freq :  It's defined the DDR3 dll bypass frequency in
-				  MHz, when ddr freq less than DRAM_DLL_DISB_FREQ,
-				  ddr3 dll will bypssed note: if dll was bypassed,
-				  the odt also stop working.
-
-- rockchip,phy_dll_disb_freq :	  Defined the PHY dll bypass frequency in
-				  MHz (Mega Hz), when ddr freq less than
-				  DRAM_DLL_DISB_FREQ, phy dll will bypssed.
-				  note: phy dll and phy odt are independent.
-
-- rockchip,ddr3_odt_disb_freq :  When dram type is DDR3, this parameter defined
-				  the odt disable frequency in MHz (Mega Hz),
-				  when ddr frequency less then ddr3_odt_disb_freq,
-				  the odt on dram side and controller side are
+				  it selects the DDR3 cl-trp-trcd type. It must be
+				  set according to "Speed Bin" in DDR3 datasheet,
+				  DO NOT use a smaller "Speed Bin" than specified
+				  for the DDR3 being used.
+
+- rockchip,pd_idle :		  Configure the PD_IDLE value. Defines the
+				  power-down idle period in which memories are
+				  placed into power-down mode if bus is idle
+				  for PD_IDLE DFI clock cycles.
+
+- rockchip,sr_idle :		  Configure the SR_IDLE value. Defines the
+				  self-refresh idle period in which memories are
+				  placed into self-refresh mode if bus is idle
+				  for SR_IDLE * 1024 DFI clock cycles (DFI
+				  clocks freq is half of DRAM clock), default
+				  value is "0".
+
+- rockchip,sr_mc_gate_idle :	  Defines the memory self-refresh and controller
+				  clock gating idle period. Memories are placed
+				  into self-refresh mode and memory controller
+				  clock arg gating started if bus is idle for
+				  sr_mc_gate_idle*1024 DFI clock cycles.
+
+- rockchip,srpd_lite_idle :	  Defines the self-refresh power down idle
+				  period in which memories are placed into
+				  self-refresh power down mode if bus is idle
+				  for srpd_lite_idle * 1024 DFI clock cycles.
+				  This parameter is for LPDDR4 only.
+
+- rockchip,standby_idle :	  Defines the standby idle period in which
+				  memories are placed into self-refresh mode.
+				  The controller, pi, PHY and DRAM clock will
+				  be gated if bus is idle for standby_idle * DFI
+				  clock cycles.
+
+- rockchip,dram_dll_disb_freq :	  Defines the DDR3 DLL bypass frequency in MHz.
+				  When DDR frequency is less than DRAM_DLL_DISB_FREQ,
+				  DDR3 DLL will be bypassed. Note: if DLL was bypassed,
+				  the odt will also stop working.
+
+- rockchip,phy_dll_disb_freq :	  Defines the PHY dll bypass frequency in
+				  MHz (Mega Hz). When DDR frequency is less than
+				  DRAM_DLL_DISB_FREQ, PHY DLL will be bypassed.
+				  Note: PHY DLL and PHY ODT are independent.
+
+- rockchip,ddr3_odt_disb_freq :	  When the DRAM type is DDR3, this parameter defines
+				  the ODT disable frequency in MHz (Mega Hz).
+				  when the DDR frequency is  less then ddr3_odt_disb_freq,
+				  the ODT on the DRAM side and controller side are
 				  both disabled.
 
-- rockchip,ddr3_drv :		  When dram type is DDR3, this parameter define
-				  the dram side driver stength in ohm, default
+- rockchip,ddr3_drv :		  When the DRAM type is DDR3, this parameter defines
+				  the DRAM side driver strength in ohms. Default
 				  value is DDR3_DS_40ohm.
 
-- rockchip,ddr3_odt :		  When dram type is DDR3, this parameter define
-				  the dram side ODT stength in ohm, default value
+- rockchip,ddr3_odt :		  When the DRAM type is DDR3, this parameter defines
+				  the DRAM side ODT strength in ohms. Default value
 				  is DDR3_ODT_120ohm.
 
-- rockchip,phy_ddr3_ca_drv :	  When dram type is DDR3, this parameter define
-				  the phy side CA line(incluing command line,
+- rockchip,phy_ddr3_ca_drv :	  When the DRAM type is DDR3, this parameter defines
+				  the phy side CA line (incluing command line,
 				  address line and clock line) driver strength.
 				  Default value is PHY_DRV_ODT_40.
 
-- rockchip,phy_ddr3_dq_drv :	  When dram type is DDR3, this parameter define
-				  the phy side DQ line(incluing DQS/DQ/DM line)
-				  driver strength. default value is PHY_DRV_ODT_40.
+- rockchip,phy_ddr3_dq_drv :	  When the DRAM type is DDR3, this parameter defines
+				  the PHY side DQ line (including DQS/DQ/DM line)
+				  driver strength. Default value is PHY_DRV_ODT_40.
 
-- rockchip,phy_ddr3_odt : 	  When dram type is DDR3, this parameter define the
-				  phy side odt strength, default value is
+- rockchip,phy_ddr3_odt : 	  When the DRAM type is DDR3, this parameter defines
+				  the PHY side ODT strength. Default value is
 				  PHY_DRV_ODT_240.
 
-- rockchip,lpddr3_odt_disb_freq : When dram type is LPDDR3, this parameter defined
-				  then odt disable frequency in MHz (Mega Hz),
-				  when ddr frequency less then ddr3_odt_disb_freq,
-				  the odt on dram side and controller side are
+- rockchip,lpddr3_odt_disb_freq : When the DRAM type is LPDDR3, this parameter defines
+				  then ODT disable frequency in MHz (Mega Hz).
+				  When DDR frequency is less then ddr3_odt_disb_freq,
+				  the ODT on the DRAM side and controller side are
 				  both disabled.
 
-- rockchip,lpddr3_drv :	  When dram type is LPDDR3, this parameter define
-				  the dram side driver stength in ohm, default
+- rockchip,lpddr3_drv :		  When the DRAM type is LPDDR3, this parameter defines
+				  the DRAM side driver strength in ohms. Default
 				  value is LP3_DS_34ohm.
 
-- rockchip,lpddr3_odt :	  When dram type is LPDDR3, this parameter define
-				  the dram side ODT stength in ohm, default value
+- rockchip,lpddr3_odt :		  When the DRAM type is LPDDR3, this parameter defines
+				  the DRAM side ODT strength in ohms. Default value
 				  is LP3_ODT_240ohm.
 
-- rockchip,phy_lpddr3_ca_drv :	  When dram type is LPDDR3, this parameter define
-				  the phy side CA line(incluing command line,
+- rockchip,phy_lpddr3_ca_drv :	  When the DRAM type is LPDDR3, this parameter defines
+				  the PHY side CA line (including command line,
 				  address line and clock line) driver strength.
-				  default value is PHY_DRV_ODT_40.
+				  Default value is PHY_DRV_ODT_40.
 
-- rockchip,phy_lpddr3_dq_drv :	  When dram type is LPDDR3, this parameter define
-				  the phy side DQ line(incluing DQS/DQ/DM line)
-				  driver strength. default value is
+- rockchip,phy_lpddr3_dq_drv :	  When the DRAM type is LPDDR3, this parameter defines
+				  the PHY side DQ line (including DQS/DQ/DM line)
+				  driver strength. Default value is
 				  PHY_DRV_ODT_40.
 
 - rockchip,phy_lpddr3_odt : 	  When dram type is LPDDR3, this parameter define
 				  the phy side odt strength, default value is
 				  PHY_DRV_ODT_240.
 
-- rockchip,lpddr4_odt_disb_freq : When dram type is LPDDR4, this parameter
-				  defined the odt disable frequency in
-				  MHz (Mega Hz), when ddr frequency less then
-				  ddr3_odt_disb_freq, the odt on dram side and
+- rockchip,lpddr4_odt_disb_freq : When the DRAM type is LPDDR4, this parameter
+				  defines the ODT disable frequency in
+				  MHz (Mega Hz). When the DDR frequency is less then
+				  ddr3_odt_disb_freq, the ODT on the DRAM side and
 				  controller side are both disabled.
 
-- rockchip,lpddr4_drv :	  When dram type is LPDDR4, this parameter define
-				  the dram side driver stength in ohm, default
+- rockchip,lpddr4_drv :		  When the DRAM type is LPDDR4, this parameter defines
+				  the DRAM side driver strength in ohms. Default
 				  value is LP4_PDDS_60ohm.
 
-- rockchip,lpddr4_dq_odt : 	  When dram type is LPDDR4, this parameter define
-				  the dram side ODT on dqs/dq line stength in ohm,
-				  default value is LP4_DQ_ODT_40ohm.
+- rockchip,lpddr4_dq_odt : 	  When the DRAM type is LPDDR4, this parameter defines
+				  the DRAM side ODT on DQS/DQ line strength in ohms.
+				  Default value is LP4_DQ_ODT_40ohm.
 
-- rockchip,lpddr4_ca_odt :	  When dram type is LPDDR4, this parameter define
-				  the dram side ODT on ca line stength in ohm,
-				  default value is LP4_CA_ODT_40ohm.
+- rockchip,lpddr4_ca_odt :	  When the DRAM type is LPDDR4, this parameter defines
+				  the DRAM side ODT on CA line strength in ohms.
+				  Default value is LP4_CA_ODT_40ohm.
 
-- rockchip,phy_lpddr4_ca_drv :	  When dram type is LPDDR4, this parameter define
-				  the phy side  CA line(incluing command address
-				  line) driver strength. default value is
+- rockchip,phy_lpddr4_ca_drv :	  When the DRAM type is LPDDR4, this parameter defines
+				  the PHY side CA line (including command address
+				  line) driver strength. Default value is
 				  PHY_DRV_ODT_40.
 
-- rockchip,phy_lpddr4_ck_cs_drv : When dram type is LPDDR4, this parameter define
-				  the phy side clock line and cs line driver
-				  strength. default value is PHY_DRV_ODT_80.
+- rockchip,phy_lpddr4_ck_cs_drv : When the DRAM type is LPDDR4, this parameter defines
+				  the PHY side clock line and CS line driver
+				  strength. Default value is PHY_DRV_ODT_80.
 
-- rockchip,phy_lpddr4_dq_drv :	  When dram type is LPDDR4, this parameter define
-				  the phy side DQ line(incluing DQS/DQ/DM line)
-				  driver strength. default value is PHY_DRV_ODT_80.
+- rockchip,phy_lpddr4_dq_drv :	  When the DRAM type is LPDDR4, this parameter defines
+				  the PHY side DQ line (including DQS/DQ/DM line)
+				  driver strength. Default value is PHY_DRV_ODT_80.
 
-- rockchip,phy_lpddr4_odt :	  When dram type is LPDDR4, this parameter define
-				  the phy side odt strength, default value is
+- rockchip,phy_lpddr4_odt :	  When the DRAM type is LPDDR4, this parameter defines
+				  the PHY side ODT strength. Default value is
 				  PHY_DRV_ODT_60.
 
 Example:
-- 
2.17.0

[PATCH 2/6] dt-bindings: clock: add DDR3 standard speed bins.

[Enric Balletbo i Serra]

DDR3 SDRAM Standard (JESD79-3F) defines some standard speed bins for
DDR3 memories. The devfreq/rk3399_dmc.txt binding refers to this file
which does not exist, so add a ddr.h file with the standard speed bins
for DDR3.

Fixes: c1ceb8f7c167 (Documentation: bindings: add dt documentation for rk3399 dmc)
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 include/dt-bindings/clock/ddr.h | 34 +++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 include/dt-bindings/clock/ddr.h

diff --git a/include/dt-bindings/clock/ddr.h b/include/dt-bindings/clock/ddr.h
new file mode 100644
index 000000000000..506aef7e609e
--- /dev/null
+++ b/include/dt-bindings/clock/ddr.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+
+#ifndef DT_BINDINGS_DDR_H
+#define DT_BINDINGS_DDR_H
+
+/* DDR3-800 Standard Speed Bins */
+#define DDR3_800D	15
+#define DDR3_800E	18
+/* DDR3-1066 Standard Speed Bins */
+#define DDR3_1066E	18
+#define DDR3_1066F	21
+#define DDR3_1066G	24
+/* DDR3-1333 Standard Speed Bins */
+#define DDR3_1333F	21
+#define DDR3_1333G	24
+#define DDR3_1333H	27
+#define DDR3_1333J	30
+/* DDR3-1600 Standard Speed Bins */
+#define DDR3_1600G	24
+#define DDR3_1600H	27
+#define DDR3_1600J	30
+#define DDR3_1600K	33
+/* DDR3-1600 Standard Speed Bins */
+#define DDR3_1866J	30
+#define DDR3_1866K	33
+#define DDR3_1866L	36
+#define DDR3_1866M	39
+/* DDR3-2133 Standard Speed Bins */
+#define DDR3_2133K	33
+#define DDR3_2133L	36
+#define DDR3_2133M	39
+#define DDR3_2133N	42
+
+#endif
-- 
2.17.0

Re: [PATCH 2/6] dt-bindings: clock: add DDR3 standard speed bins.

[Heiko Stuebner]

Hi Enric,

Am Donnerstag, 19. April 2018, 12:40:15 CEST schrieb Enric Balletbo i Serra:
> DDR3 SDRAM Standard (JESD79-3F) defines some standard speed bins for
> DDR3 memories. The devfreq/rk3399_dmc.txt binding refers to this file
> which does not exist, so add a ddr.h file with the standard speed bins
> for DDR3.
> 
> Fixes: c1ceb8f7c167 (Documentation: bindings: add dt documentation for rk3399 dmc)
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  include/dt-bindings/clock/ddr.h | 34 +++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 include/dt-bindings/clock/ddr.h
> 
> diff --git a/include/dt-bindings/clock/ddr.h b/include/dt-bindings/clock/ddr.h
> new file mode 100644
> index 000000000000..506aef7e609e
> --- /dev/null
> +++ b/include/dt-bindings/clock/ddr.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +
> +#ifndef DT_BINDINGS_DDR_H
> +#define DT_BINDINGS_DDR_H
> +
> +/* DDR3-800 Standard Speed Bins */
> +#define DDR3_800D	15
> +#define DDR3_800E		18
> +/* DDR3-1066 Standard Speed Bins */
> +#define DDR3_1066E	18
> +#define DDR3_1066F	21
> +#define DDR3_1066G	24

looking at the mentioned jedec standard, I don't see where these numerical
values are defined in the standard itself. [I may be blind though]

Could you explain a bit more where these numerical values are coming from?


Thanks
Heiko

Re: [PATCH 2/6] dt-bindings: clock: add DDR3 standard speed bins.

[Enric Balletbo i Serra]

Hi Heiko,

On 19/04/18 13:10, Heiko Stuebner wrote:
> Hi Enric,
> 
> Am Donnerstag, 19. April 2018, 12:40:15 CEST schrieb Enric Balletbo i Serra:
>> DDR3 SDRAM Standard (JESD79-3F) defines some standard speed bins for
>> DDR3 memories. The devfreq/rk3399_dmc.txt binding refers to this file
>> which does not exist, so add a ddr.h file with the standard speed bins
>> for DDR3.
>>
>> Fixes: c1ceb8f7c167 (Documentation: bindings: add dt documentation for rk3399 dmc)
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>>  include/dt-bindings/clock/ddr.h | 34 +++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>  create mode 100644 include/dt-bindings/clock/ddr.h
>>
>> diff --git a/include/dt-bindings/clock/ddr.h b/include/dt-bindings/clock/ddr.h
>> new file mode 100644
>> index 000000000000..506aef7e609e
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/ddr.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
>> +
>> +#ifndef DT_BINDINGS_DDR_H
>> +#define DT_BINDINGS_DDR_H
>> +
>> +/* DDR3-800 Standard Speed Bins */
>> +#define DDR3_800D	15
>> +#define DDR3_800E		18
>> +/* DDR3-1066 Standard Speed Bins */
>> +#define DDR3_1066E	18
>> +#define DDR3_1066F	21
>> +#define DDR3_1066G	24
> 
> looking at the mentioned jedec standard, I don't see where these numerical
> values are defined in the standard itself. [I may be blind though]
> 

Damm it. No, you're not blind. I did an horrible mistake adding an old version
of this file. It's wrong, that is supposed to be there is DDR3 table available
in the ATF [1] not this.

        /* 5-5-5 */
	DDR3_800D = 0,
	/* 6-6-6 */
	DDR3_800E = 1,
	/* 6-6-6 */
	DDR3_1066E = 2,
	/* 7-7-7 */
	DDR3_1066F = 3,
        ...

The value is passed directly to the ATF so the above values are the correct.

One question that I have on my mind is if this file should be called as is or if
will be better name as atf-ddr.h or something like that.
ddr.h -> atf-ddr.h

[1]
https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/rockchip/rk3399/drivers/dram/dram_spec_timing.h

Sorry about that. Guess it's needed a v2 but let's wait a little bit for if more
discussion arises.

Best regards,
  Enric

> Could you explain a bit more where these numerical values are coming from?
> 
> 
> Thanks
> Heiko
> 

Re: [PATCH 2/6] dt-bindings: clock: add DDR3 standard speed bins.

[Heiko Stuebner]

Hi Enric,

Am Donnerstag, 19. April 2018, 13:30:16 CEST schrieb Enric Balletbo i Serra:
> On 19/04/18 13:10, Heiko Stuebner wrote:
> > Am Donnerstag, 19. April 2018, 12:40:15 CEST schrieb Enric Balletbo i Serra:
> >> DDR3 SDRAM Standard (JESD79-3F) defines some standard speed bins for
> >> DDR3 memories. The devfreq/rk3399_dmc.txt binding refers to this file
> >> which does not exist, so add a ddr.h file with the standard speed bins
> >> for DDR3.
> >>
> >> Fixes: c1ceb8f7c167 (Documentation: bindings: add dt documentation for rk3399 dmc)
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >>  include/dt-bindings/clock/ddr.h | 34 +++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>  create mode 100644 include/dt-bindings/clock/ddr.h
> >>
> >> diff --git a/include/dt-bindings/clock/ddr.h b/include/dt-bindings/clock/ddr.h
> >> new file mode 100644
> >> index 000000000000..506aef7e609e
> >> --- /dev/null
> >> +++ b/include/dt-bindings/clock/ddr.h
> >> @@ -0,0 +1,34 @@
> >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> >> +
> >> +#ifndef DT_BINDINGS_DDR_H
> >> +#define DT_BINDINGS_DDR_H
> >> +
> >> +/* DDR3-800 Standard Speed Bins */
> >> +#define DDR3_800D	15
> >> +#define DDR3_800E		18
> >> +/* DDR3-1066 Standard Speed Bins */
> >> +#define DDR3_1066E	18
> >> +#define DDR3_1066F	21
> >> +#define DDR3_1066G	24
> > 
> > looking at the mentioned jedec standard, I don't see where these numerical
> > values are defined in the standard itself. [I may be blind though]
> > 
> 
> Damm it. No, you're not blind. I did an horrible mistake adding an old version
> of this file. It's wrong, that is supposed to be there is DDR3 table available
> in the ATF [1] not this.

Mistakes can always happen ... so no problem :-) .


>         /* 5-5-5 */
> 	DDR3_800D = 0,
> 	/* 6-6-6 */
> 	DDR3_800E = 1,
> 	/* 6-6-6 */
> 	DDR3_1066E = 2,
> 	/* 7-7-7 */
> 	DDR3_1066F = 3,
>         ...
> 
> The value is passed directly to the ATF so the above values are the correct.
> 
> One question that I have on my mind is if this file should be called as is or if
> will be better name as atf-ddr.h or something like that.
> ddr.h -> atf-ddr.h

The more interesting question would be if this isn't a very much rockchip
or even rk3399-specific scheme. So such a generic header name looks very
much out of place (also including the very generic constant names).

So far the only upstream ATF implementation is for the rk3399.


Heiko

> 
> [1]
> https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/rockchip/rk3399/drivers/dram/dram_spec_timing.h
>
> Sorry about that. Guess it's needed a v2 but let's wait a little bit for if more
> discussion arises.
> 
> Best regards,
>   Enric
> 
> > Could you explain a bit more where these numerical values are coming from?
> > 
> > 
> > Thanks
> > Heiko
> > 
> 
> 

[PATCH 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[Enric Balletbo i Serra]

We have already wait dcf done in ATF, so don't need wait dcf irq
in kernel, besides, clear dcf irq in kernel will import competiton
between kernel and ATF, only handle dcf irq in ATF is a better way.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/devfreq/rk3399_dmc.c | 53 +-----------------------------------
 1 file changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 5dfbfa3cc878..44a379657cd5 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -68,15 +68,6 @@ struct rk3399_dmcfreq {
 	struct devfreq_event_dev *edev;
 	struct mutex lock;
 	struct dram_timing timing;
-
-	/*
-	 * DDR Converser of Frequency (DCF) is used to implement DDR frequency
-	 * conversion without the participation of CPU, we will implement and
-	 * control it in arm trust firmware.
-	 */
-	wait_queue_head_t	wait_dcf_queue;
-	int irq;
-	int wait_dcf_flag;
 	struct regulator *vdd_center;
 	unsigned long rate, target_rate;
 	unsigned long volt, target_volt;
@@ -117,7 +108,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 			goto out;
 		}
 	}
-	dmcfreq->wait_dcf_flag = 1;
 
 	err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
 	if (err) {
@@ -128,14 +118,6 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 		goto out;
 	}
 
-	/*
-	 * Wait until bcf irq happen, it means freq scaling finish in
-	 * arm trust firmware, use 100ms as timeout time.
-	 */
-	if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
-				!dmcfreq->wait_dcf_flag, HZ / 10))
-		dev_warn(dev, "Timeout waiting for dcf interrupt\n");
-
 	/*
 	 * Check the dpll rate,
 	 * There only two result we will get,
@@ -241,22 +223,6 @@ static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend,
 			 rk3399_dmcfreq_resume);
 
-static irqreturn_t rk3399_dmc_irq(int irq, void *dev_id)
-{
-	struct rk3399_dmcfreq *dmcfreq = dev_id;
-	struct arm_smccc_res res;
-
-	dmcfreq->wait_dcf_flag = 0;
-	wake_up(&dmcfreq->wait_dcf_queue);
-
-	/* Clear the DCF interrupt */
-	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0,
-		      ROCKCHIP_SIP_CONFIG_DRAM_CLR_IRQ,
-		      0, 0, 0, 0, &res);
-
-	return IRQ_HANDLED;
-}
-
 static int of_get_ddr_timings(struct dram_timing *timing,
 			      struct device_node *np)
 {
@@ -330,16 +296,10 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = pdev->dev.of_node;
 	struct rk3399_dmcfreq *data;
-	int ret, irq, index, size;
+	int ret, index, size;
 	uint32_t *timing;
 	struct dev_pm_opp *opp;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		dev_err(&pdev->dev,
-			"Cannot get the dmc interrupt resource: %d\n", irq);
-		return irq;
-	}
 	data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -358,17 +318,6 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 		return PTR_ERR(data->dmc_clk);
 	};
 
-	data->irq = irq;
-	ret = devm_request_irq(dev, irq, rk3399_dmc_irq, 0,
-			       dev_name(dev), data);
-	if (ret) {
-		dev_err(dev, "Failed to request dmc irq: %d\n", ret);
-		return ret;
-	}
-
-	init_waitqueue_head(&data->wait_dcf_queue);
-	data->wait_dcf_flag = 0;
-
 	data->edev = devfreq_event_get_edev_by_phandle(dev, 0);
 	if (IS_ERR(data->edev))
 		return -EPROBE_DEFER;
-- 
2.17.0

Re: [PATCH 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[Chanwoo Choi]

Hi,

On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote:
> We have already wait dcf done in ATF, so don't need wait dcf irq
> in kernel, besides, clear dcf irq in kernel will import competiton
> between kernel and ATF, only handle dcf irq in ATF is a better way.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---

The dcf_irq depends on the architecture and device. It means that
the author or other people access the confidential document
have to guarantee this change. So, Lin Huang is one of author of this patch,
looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

[snip]

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH 4/6] dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not required.

[Enric Balletbo i Serra]

In ATF we already wait for DDR dvfs finish, so don't need to do this in
kernel, so remove the interrupts properties as is not longer required.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
index d83ef821d282..e5307155e901 100644
--- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
+++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
@@ -5,10 +5,6 @@ Required properties:
 - devfreq-events:	 Node to get DDR loading, Refer to
 			 Documentation/devicetree/bindings/devfreq/event/
 			 rockchip-dfi.txt
-- interrupts:		 The CPU interrupt number. The interrupt specifier
-			 format depends on the interrupt controller.
-			 It should be a DCF interrupt. When DDR DVFS finishes
-			 a DCF interrupt is triggered.
 - clocks:		 Phandles for clock specified in "clock-names" property
 - clock-names :		 The name of clock used by the DFI, must be
 			 "pclk_ddr_mon";
@@ -172,7 +168,6 @@ Example:
 	dmc: dmc {
 		compatible = "rockchip,rk3399-dmc";
 		devfreq-events = <&dfi>;
-		interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru SCLK_DDRCLK>;
 		clock-names = "dmc_clk";
 		operating-points-v2 = <&dmc_opp_table>;
-- 
2.17.0

Re: [PATCH 4/6] dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not required.

[Chanwoo Choi]

Hi,

On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote:
> In ATF we already wait for DDR dvfs finish, so don't need to do this in
> kernel, so remove the interrupts properties as is not longer required.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> index d83ef821d282..e5307155e901 100644
> --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> @@ -5,10 +5,6 @@ Required properties:
>  - devfreq-events:	 Node to get DDR loading, Refer to
>  			 Documentation/devicetree/bindings/devfreq/event/
>  			 rockchip-dfi.txt
> -- interrupts:		 The CPU interrupt number. The interrupt specifier
> -			 format depends on the interrupt controller.
> -			 It should be a DCF interrupt. When DDR DVFS finishes
> -			 a DCF interrupt is triggered.
>  - clocks:		 Phandles for clock specified in "clock-names" property
>  - clock-names :		 The name of clock used by the DFI, must be
>  			 "pclk_ddr_mon";
> @@ -172,7 +168,6 @@ Example:
>  	dmc: dmc {
>  		compatible = "rockchip,rk3399-dmc";
>  		devfreq-events = <&dfi>;
> -		interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&cru SCLK_DDRCLK>;
>  		clock-names = "dmc_clk";
>  		operating-points-v2 = <&dmc_opp_table>;
> 

The patch3[1] removes the code related to irq. So, Looks good to me.
[1] "[PATCH 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event."

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[Enric Balletbo i Serra]

From: Lin Huang <hl@rock-chips.com>

We just return -EPROBE_DEFER error code to caller and do not
print error message when try to get center logic regulator
and DMC clock defer.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/devfreq/rk3399_dmc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 44a379657cd5..5bfca028eaaf 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 
 	data->vdd_center = devm_regulator_get(dev, "center");
 	if (IS_ERR(data->vdd_center)) {
+		if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
 		dev_err(dev, "Cannot get the regulator \"center\"\n");
 		return PTR_ERR(data->vdd_center);
 	}
 
 	data->dmc_clk = devm_clk_get(dev, "dmc_clk");
 	if (IS_ERR(data->dmc_clk)) {
+		if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
 		dev_err(dev, "Cannot get the clk dmc_clk\n");
 		return PTR_ERR(data->dmc_clk);
 	};
-- 
2.17.0

Re: [PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[Ulf Hansson]

On 19 April 2018 at 12:40, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> From: Lin Huang <hl@rock-chips.com>
>
> We just return -EPROBE_DEFER error code to caller and do not
> print error message when try to get center logic regulator
> and DMC clock defer.
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
>  drivers/devfreq/rk3399_dmc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 44a379657cd5..5bfca028eaaf 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>
>         data->vdd_center = devm_regulator_get(dev, "center");
>         if (IS_ERR(data->vdd_center)) {
> +               if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER)
> +                       return -EPROBE_DEFER;
> +
>                 dev_err(dev, "Cannot get the regulator \"center\"\n");

Doesn't the clock core already print an error message for this?

Maybe a better way is simply to drop the printing instead of trying to
have a special case for it?

>                 return PTR_ERR(data->vdd_center);
>         }
>
>         data->dmc_clk = devm_clk_get(dev, "dmc_clk");
>         if (IS_ERR(data->dmc_clk)) {
> +               if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER)
> +                       return -EPROBE_DEFER;
> +
>                 dev_err(dev, "Cannot get the clk dmc_clk\n");
>                 return PTR_ERR(data->dmc_clk);
>         };
> --
> 2.17.0
>

Kind regards
Uffe

Re: [PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[Ezequiel Garcia]

On Mon, 2018-04-23 at 12:44 +0200, Ulf Hansson wrote:
> On 19 April 2018 at 12:40, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> > From: Lin Huang <hl@rock-chips.com>
> > 
> > We just return -EPROBE_DEFER error code to caller and do not
> > print error message when try to get center logic regulator
> > and DMC clock defer.
> > 
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > ---
> > 
> >  drivers/devfreq/rk3399_dmc.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> > index 44a379657cd5..5bfca028eaaf 100644
> > --- a/drivers/devfreq/rk3399_dmc.c
> > +++ b/drivers/devfreq/rk3399_dmc.c
> > @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
> > 
> >         data->vdd_center = devm_regulator_get(dev, "center");
> >         if (IS_ERR(data->vdd_center)) {
> > +               if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER)
> > +                       return -EPROBE_DEFER;
> > +
> >                 dev_err(dev, "Cannot get the regulator \"center\"\n");
> 
> Doesn't the clock core already print an error message for this?
> 

s/clock/regulator?

> Maybe a better way is simply to drop the printing instead of trying to
> have a special case for it?
> 

I don't think so.

If you remove this print you: i) might get a print from the core,
or ii) maybe not because of some path without a print.

And even if you do get an error, it might not relate exactly to the
driver that requested the resource, because you might be printing
via pr_xxx, from a context without a struct dev to use dev_xxx.

> >                 return PTR_ERR(data->vdd_center);
> >         }
> > 
> >         data->dmc_clk = devm_clk_get(dev, "dmc_clk");
> >         if (IS_ERR(data->dmc_clk)) {
> > +               if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER)
> > +                       return -EPROBE_DEFER;
> > +
> >                 dev_err(dev, "Cannot get the clk dmc_clk\n");
> >                 return PTR_ERR(data->dmc_clk);
> >         };
> > --
> > 2.17.0
> > 
> 
> Kind regards
> Uffe
> 

Re: [PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[Chanwoo Choi]

On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
> 
> We just return -EPROBE_DEFER error code to caller and do not
> print error message when try to get center logic regulator
> and DMC clock defer.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 44a379657cd5..5bfca028eaaf 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -308,12 +308,18 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  
>  	data->vdd_center = devm_regulator_get(dev, "center");
>  	if (IS_ERR(data->vdd_center)) {
> +		if (PTR_ERR(data->vdd_center) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
>  		dev_err(dev, "Cannot get the regulator \"center\"\n");
>  		return PTR_ERR(data->vdd_center);
>  	}
>  
>  	data->dmc_clk = devm_clk_get(dev, "dmc_clk");
>  	if (IS_ERR(data->dmc_clk)) {
> +		if (PTR_ERR(data->dmc_clk) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
>  		dev_err(dev, "Cannot get the clk dmc_clk\n");
>  		return PTR_ERR(data->dmc_clk);
>  	};
> 

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Enric Balletbo i Serra]

From: Lin Huang <hl@rock-chips.com>

Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
ensure that the pd driver and the dmc driver will not access at this
register at the same time.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
 drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
 include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 45 deletions(-)
 create mode 100644 include/soc/rockchip/rk3399_dmc.h

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 5bfca028eaaf..a1f320634d69 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -27,51 +27,7 @@
 #include <linux/suspend.h>
 
 #include <soc/rockchip/rockchip_sip.h>
-
-struct dram_timing {
-	unsigned int ddr3_speed_bin;
-	unsigned int pd_idle;
-	unsigned int sr_idle;
-	unsigned int sr_mc_gate_idle;
-	unsigned int srpd_lite_idle;
-	unsigned int standby_idle;
-	unsigned int auto_pd_dis_freq;
-	unsigned int dram_dll_dis_freq;
-	unsigned int phy_dll_dis_freq;
-	unsigned int ddr3_odt_dis_freq;
-	unsigned int ddr3_drv;
-	unsigned int ddr3_odt;
-	unsigned int phy_ddr3_ca_drv;
-	unsigned int phy_ddr3_dq_drv;
-	unsigned int phy_ddr3_odt;
-	unsigned int lpddr3_odt_dis_freq;
-	unsigned int lpddr3_drv;
-	unsigned int lpddr3_odt;
-	unsigned int phy_lpddr3_ca_drv;
-	unsigned int phy_lpddr3_dq_drv;
-	unsigned int phy_lpddr3_odt;
-	unsigned int lpddr4_odt_dis_freq;
-	unsigned int lpddr4_drv;
-	unsigned int lpddr4_dq_odt;
-	unsigned int lpddr4_ca_odt;
-	unsigned int phy_lpddr4_ca_drv;
-	unsigned int phy_lpddr4_ck_cs_drv;
-	unsigned int phy_lpddr4_dq_drv;
-	unsigned int phy_lpddr4_odt;
-};
-
-struct rk3399_dmcfreq {
-	struct device *dev;
-	struct devfreq *devfreq;
-	struct devfreq_simple_ondemand_data ondemand_data;
-	struct clk *dmc_clk;
-	struct devfreq_event_dev *edev;
-	struct mutex lock;
-	struct dram_timing timing;
-	struct regulator *vdd_center;
-	unsigned long rate, target_rate;
-	unsigned long volt, target_volt;
-};
+#include <soc/rockchip/rk3399_dmc.h>
 
 static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 				 u32 flags)
@@ -394,6 +350,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 
 	data->dev = dev;
 	platform_set_drvdata(pdev, data);
+	pd_register_notify_to_dmc(data->devfreq);
 
 	return 0;
 }
diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 53efc386b1ad..7acc836e7eb7 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -8,6 +8,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/devfreq.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/err.h>
@@ -76,9 +77,13 @@ struct rockchip_pmu {
 	const struct rockchip_pmu_info *info;
 	struct mutex mutex; /* mutex lock for pmu */
 	struct genpd_onecell_data genpd_data;
+	struct devfreq *devfreq;
+	struct notifier_block dmc_nb;
 	struct generic_pm_domain *domains[];
 };
 
+static struct rockchip_pmu *dmc_pmu;
+
 #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
 
 #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
@@ -601,6 +606,30 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 	return error;
 }
 
+static int dmc_notify(struct notifier_block *nb, unsigned long event,
+		      void *data)
+{
+	if (event == DEVFREQ_PRECHANGE)
+		mutex_lock(&dmc_pmu->mutex);
+	else if (event == DEVFREQ_POSTCHANGE)
+		mutex_unlock(&dmc_pmu->mutex);
+
+	return NOTIFY_OK;
+}
+
+int pd_register_notify_to_dmc(struct devfreq *devfreq)
+{
+	if (!dmc_pmu)
+		return -EPROBE_DEFER;
+
+	dmc_pmu->devfreq = devfreq;
+	dmc_pmu->dmc_nb.notifier_call = dmc_notify;
+	devfreq_register_notifier(dmc_pmu->devfreq, &dmc_pmu->dmc_nb,
+				  DEVFREQ_TRANSITION_NOTIFIER);
+	return 0;
+}
+EXPORT_SYMBOL(pd_register_notify_to_dmc);
+
 static int rockchip_pm_domain_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -694,6 +723,8 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 		goto err_out;
 	}
 
+	dmc_pmu = pmu;
+
 	return 0;
 
 err_out:
diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
new file mode 100644
index 000000000000..7ccdfff1a154
--- /dev/null
+++ b/include/soc/rockchip/rk3399_dmc.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * DMC (Dynamic Memory Controller) for RK3399 - Definitions
+ *
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Author: Lin Huang <hl@rock-chips.com>
+ */
+
+#ifndef __SOC_RK3399_DMC_H
+#define __SOC_RK3399_DMC_H
+
+#include <linux/devfreq.h>
+
+struct dram_timing {
+	unsigned int ddr3_speed_bin;
+	unsigned int pd_idle;
+	unsigned int sr_idle;
+	unsigned int sr_mc_gate_idle;
+	unsigned int srpd_lite_idle;
+	unsigned int standby_idle;
+	unsigned int auto_pd_dis_freq;
+	unsigned int dram_dll_dis_freq;
+	unsigned int phy_dll_dis_freq;
+	unsigned int ddr3_odt_dis_freq;
+	unsigned int ddr3_drv;
+	unsigned int ddr3_odt;
+	unsigned int phy_ddr3_ca_drv;
+	unsigned int phy_ddr3_dq_drv;
+	unsigned int phy_ddr3_odt;
+	unsigned int lpddr3_odt_dis_freq;
+	unsigned int lpddr3_drv;
+	unsigned int lpddr3_odt;
+	unsigned int phy_lpddr3_ca_drv;
+	unsigned int phy_lpddr3_dq_drv;
+	unsigned int phy_lpddr3_odt;
+	unsigned int lpddr4_odt_dis_freq;
+	unsigned int lpddr4_drv;
+	unsigned int lpddr4_dq_odt;
+	unsigned int lpddr4_ca_odt;
+	unsigned int phy_lpddr4_ca_drv;
+	unsigned int phy_lpddr4_ck_cs_drv;
+	unsigned int phy_lpddr4_dq_drv;
+	unsigned int phy_lpddr4_odt;
+};
+
+struct rk3399_dmcfreq {
+	struct device *dev;
+	struct devfreq *devfreq;
+	struct devfreq_simple_ondemand_data ondemand_data;
+	struct clk *dmc_clk;
+	struct devfreq_event_dev *edev;
+	struct mutex lock;
+	struct dram_timing timing;
+	struct regulator *vdd_center;
+	unsigned long rate, target_rate;
+	unsigned long volt, target_volt;
+	struct dev_pm_opp *curr_opp;
+};
+
+int pd_register_notify_to_dmc(struct devfreq *devfreq);
+
+#endif
-- 
2.17.0

[PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Enric Balletbo i Serra]

From: Lin Huang <hl@rock-chips.com>

Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
ensure that the pd driver and the dmc driver will not access at this
register at the same time.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
 drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
 include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 45 deletions(-)
 create mode 100644 include/soc/rockchip/rk3399_dmc.h

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 5bfca028eaaf..a1f320634d69 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -27,51 +27,7 @@
 #include <linux/suspend.h>
 
 #include <soc/rockchip/rockchip_sip.h>
-
-struct dram_timing {
-	unsigned int ddr3_speed_bin;
-	unsigned int pd_idle;
-	unsigned int sr_idle;
-	unsigned int sr_mc_gate_idle;
-	unsigned int srpd_lite_idle;
-	unsigned int standby_idle;
-	unsigned int auto_pd_dis_freq;
-	unsigned int dram_dll_dis_freq;
-	unsigned int phy_dll_dis_freq;
-	unsigned int ddr3_odt_dis_freq;
-	unsigned int ddr3_drv;
-	unsigned int ddr3_odt;
-	unsigned int phy_ddr3_ca_drv;
-	unsigned int phy_ddr3_dq_drv;
-	unsigned int phy_ddr3_odt;
-	unsigned int lpddr3_odt_dis_freq;
-	unsigned int lpddr3_drv;
-	unsigned int lpddr3_odt;
-	unsigned int phy_lpddr3_ca_drv;
-	unsigned int phy_lpddr3_dq_drv;
-	unsigned int phy_lpddr3_odt;
-	unsigned int lpddr4_odt_dis_freq;
-	unsigned int lpddr4_drv;
-	unsigned int lpddr4_dq_odt;
-	unsigned int lpddr4_ca_odt;
-	unsigned int phy_lpddr4_ca_drv;
-	unsigned int phy_lpddr4_ck_cs_drv;
-	unsigned int phy_lpddr4_dq_drv;
-	unsigned int phy_lpddr4_odt;
-};
-
-struct rk3399_dmcfreq {
-	struct device *dev;
-	struct devfreq *devfreq;
-	struct devfreq_simple_ondemand_data ondemand_data;
-	struct clk *dmc_clk;
-	struct devfreq_event_dev *edev;
-	struct mutex lock;
-	struct dram_timing timing;
-	struct regulator *vdd_center;
-	unsigned long rate, target_rate;
-	unsigned long volt, target_volt;
-};
+#include <soc/rockchip/rk3399_dmc.h>
 
 static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 				 u32 flags)
@@ -394,6 +350,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 
 	data->dev = dev;
 	platform_set_drvdata(pdev, data);
+	pd_register_notify_to_dmc(data->devfreq);
 
 	return 0;
 }
diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 53efc386b1ad..7acc836e7eb7 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -8,6 +8,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/devfreq.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/err.h>
@@ -76,9 +77,13 @@ struct rockchip_pmu {
 	const struct rockchip_pmu_info *info;
 	struct mutex mutex; /* mutex lock for pmu */
 	struct genpd_onecell_data genpd_data;
+	struct devfreq *devfreq;
+	struct notifier_block dmc_nb;
 	struct generic_pm_domain *domains[];
 };
 
+static struct rockchip_pmu *dmc_pmu;
+
 #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
 
 #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
@@ -601,6 +606,30 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 	return error;
 }
 
+static int dmc_notify(struct notifier_block *nb, unsigned long event,
+		      void *data)
+{
+	if (event == DEVFREQ_PRECHANGE)
+		mutex_lock(&dmc_pmu->mutex);
+	else if (event == DEVFREQ_POSTCHANGE)
+		mutex_unlock(&dmc_pmu->mutex);
+
+	return NOTIFY_OK;
+}
+
+int pd_register_notify_to_dmc(struct devfreq *devfreq)
+{
+	if (!dmc_pmu)
+		return -EPROBE_DEFER;
+
+	dmc_pmu->devfreq = devfreq;
+	dmc_pmu->dmc_nb.notifier_call = dmc_notify;
+	devfreq_register_notifier(dmc_pmu->devfreq, &dmc_pmu->dmc_nb,
+				  DEVFREQ_TRANSITION_NOTIFIER);
+	return 0;
+}
+EXPORT_SYMBOL(pd_register_notify_to_dmc);
+
 static int rockchip_pm_domain_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -694,6 +723,8 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 		goto err_out;
 	}
 
+	dmc_pmu = pmu;
+
 	return 0;
 
 err_out:
diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
new file mode 100644
index 000000000000..7ccdfff1a154
--- /dev/null
+++ b/include/soc/rockchip/rk3399_dmc.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * DMC (Dynamic Memory Controller) for RK3399 - Definitions
+ *
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Author: Lin Huang <hl@rock-chips.com>
+ */
+
+#ifndef __SOC_RK3399_DMC_H
+#define __SOC_RK3399_DMC_H
+
+#include <linux/devfreq.h>
+
+struct dram_timing {
+	unsigned int ddr3_speed_bin;
+	unsigned int pd_idle;
+	unsigned int sr_idle;
+	unsigned int sr_mc_gate_idle;
+	unsigned int srpd_lite_idle;
+	unsigned int standby_idle;
+	unsigned int auto_pd_dis_freq;
+	unsigned int dram_dll_dis_freq;
+	unsigned int phy_dll_dis_freq;
+	unsigned int ddr3_odt_dis_freq;
+	unsigned int ddr3_drv;
+	unsigned int ddr3_odt;
+	unsigned int phy_ddr3_ca_drv;
+	unsigned int phy_ddr3_dq_drv;
+	unsigned int phy_ddr3_odt;
+	unsigned int lpddr3_odt_dis_freq;
+	unsigned int lpddr3_drv;
+	unsigned int lpddr3_odt;
+	unsigned int phy_lpddr3_ca_drv;
+	unsigned int phy_lpddr3_dq_drv;
+	unsigned int phy_lpddr3_odt;
+	unsigned int lpddr4_odt_dis_freq;
+	unsigned int lpddr4_drv;
+	unsigned int lpddr4_dq_odt;
+	unsigned int lpddr4_ca_odt;
+	unsigned int phy_lpddr4_ca_drv;
+	unsigned int phy_lpddr4_ck_cs_drv;
+	unsigned int phy_lpddr4_dq_drv;
+	unsigned int phy_lpddr4_odt;
+};
+
+struct rk3399_dmcfreq {
+	struct device *dev;
+	struct devfreq *devfreq;
+	struct devfreq_simple_ondemand_data ondemand_data;
+	struct clk *dmc_clk;
+	struct devfreq_event_dev *edev;
+	struct mutex lock;
+	struct dram_timing timing;
+	struct regulator *vdd_center;
+	unsigned long rate, target_rate;
+	unsigned long volt, target_volt;
+	struct dev_pm_opp *curr_opp;
+};
+
+int pd_register_notify_to_dmc(struct devfreq *devfreq);
+
+#endif
-- 
2.17.0

Re: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Chanwoo Choi]

Hi,

On 2018년 04월 19일 19:40, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
> 
> Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
> ensure that the pd driver and the dmc driver will not access at this
> register at the same time.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
>  drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
>  include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+), 45 deletions(-)
>  create mode 100644 include/soc/rockchip/rk3399_dmc.h
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 5bfca028eaaf..a1f320634d69 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -27,51 +27,7 @@
>  #include <linux/suspend.h>
>  
>  #include <soc/rockchip/rockchip_sip.h>
> -
> -struct dram_timing {
> -	unsigned int ddr3_speed_bin;
> -	unsigned int pd_idle;
> -	unsigned int sr_idle;
> -	unsigned int sr_mc_gate_idle;
> -	unsigned int srpd_lite_idle;
> -	unsigned int standby_idle;
> -	unsigned int auto_pd_dis_freq;
> -	unsigned int dram_dll_dis_freq;
> -	unsigned int phy_dll_dis_freq;
> -	unsigned int ddr3_odt_dis_freq;
> -	unsigned int ddr3_drv;
> -	unsigned int ddr3_odt;
> -	unsigned int phy_ddr3_ca_drv;
> -	unsigned int phy_ddr3_dq_drv;
> -	unsigned int phy_ddr3_odt;
> -	unsigned int lpddr3_odt_dis_freq;
> -	unsigned int lpddr3_drv;
> -	unsigned int lpddr3_odt;
> -	unsigned int phy_lpddr3_ca_drv;
> -	unsigned int phy_lpddr3_dq_drv;
> -	unsigned int phy_lpddr3_odt;
> -	unsigned int lpddr4_odt_dis_freq;
> -	unsigned int lpddr4_drv;
> -	unsigned int lpddr4_dq_odt;
> -	unsigned int lpddr4_ca_odt;
> -	unsigned int phy_lpddr4_ca_drv;
> -	unsigned int phy_lpddr4_ck_cs_drv;
> -	unsigned int phy_lpddr4_dq_drv;
> -	unsigned int phy_lpddr4_odt;
> -};
> -
> -struct rk3399_dmcfreq {
> -	struct device *dev;
> -	struct devfreq *devfreq;
> -	struct devfreq_simple_ondemand_data ondemand_data;
> -	struct clk *dmc_clk;
> -	struct devfreq_event_dev *edev;
> -	struct mutex lock;
> -	struct dram_timing timing;
> -	struct regulator *vdd_center;
> -	unsigned long rate, target_rate;
> -	unsigned long volt, target_volt;
> -};
> +#include <soc/rockchip/rk3399_dmc.h>
>  
>  static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>  				 u32 flags)
> @@ -394,6 +350,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  
>  	data->dev = dev;
>  	platform_set_drvdata(pdev, data);
> +	pd_register_notify_to_dmc(data->devfreq);
>  
>  	return 0;
>  }
> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
> index 53efc386b1ad..7acc836e7eb7 100644
> --- a/drivers/soc/rockchip/pm_domains.c
> +++ b/drivers/soc/rockchip/pm_domains.c
> @@ -8,6 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/devfreq.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/err.h>
> @@ -76,9 +77,13 @@ struct rockchip_pmu {
>  	const struct rockchip_pmu_info *info;
>  	struct mutex mutex; /* mutex lock for pmu */
>  	struct genpd_onecell_data genpd_data;
> +	struct devfreq *devfreq;
> +	struct notifier_block dmc_nb;
>  	struct generic_pm_domain *domains[];
>  };
>  
> +static struct rockchip_pmu *dmc_pmu;
> +
>  #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
>  
>  #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
> @@ -601,6 +606,30 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  	return error;
>  }
>  
> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
> +		      void *data)
> +{
> +	if (event == DEVFREQ_PRECHANGE)
> +		mutex_lock(&dmc_pmu->mutex);
> +	else if (event == DEVFREQ_POSTCHANGE)
> +		mutex_unlock(&dmc_pmu->mutex);
> +
> +	return NOTIFY_OK;
> +}
> +
> +int pd_register_notify_to_dmc(struct devfreq *devfreq)
> +{
> +	if (!dmc_pmu)
> +		return -EPROBE_DEFER;
> +
> +	dmc_pmu->devfreq = devfreq;
> +	dmc_pmu->dmc_nb.notifier_call = dmc_notify;
> +	devfreq_register_notifier(dmc_pmu->devfreq, &dmc_pmu->dmc_nb,
> +				  DEVFREQ_TRANSITION_NOTIFIER);
> +	return 0;
> +}
> +EXPORT_SYMBOL(pd_register_notify_to_dmc);

I think that it is not proper to define the nonstandard function
for only specific device driver. Maybe, It makes the code more complicated.
Between linux kernel frameworks, we have to use the defined function
by linux kernel frameworks.

If drivers/soc/rockchip/pm_domains.c is able to get the devfreq instance
through devicetree, the exported function is not necessary. Sorry for that
I'm not sure the alternative.

[snip]

> diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
> new file mode 100644
> index 000000000000..7ccdfff1a154
> --- /dev/null
> +++ b/include/soc/rockchip/rk3399_dmc.h
> @@ -0,0 +1,63 @@

[snip]

> +
> +int pd_register_notify_to_dmc(struct devfreq *devfreq);
> +
> +#endif
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Chanwoo Choi]

Hi,

On 2018? 04? 19? 19:40, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
> 
> Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
> ensure that the pd driver and the dmc driver will not access at this
> register at the same time.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
>  drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
>  include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
>  3 files changed, 96 insertions(+), 45 deletions(-)
>  create mode 100644 include/soc/rockchip/rk3399_dmc.h
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 5bfca028eaaf..a1f320634d69 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -27,51 +27,7 @@
>  #include <linux/suspend.h>
>  
>  #include <soc/rockchip/rockchip_sip.h>
> -
> -struct dram_timing {
> -	unsigned int ddr3_speed_bin;
> -	unsigned int pd_idle;
> -	unsigned int sr_idle;
> -	unsigned int sr_mc_gate_idle;
> -	unsigned int srpd_lite_idle;
> -	unsigned int standby_idle;
> -	unsigned int auto_pd_dis_freq;
> -	unsigned int dram_dll_dis_freq;
> -	unsigned int phy_dll_dis_freq;
> -	unsigned int ddr3_odt_dis_freq;
> -	unsigned int ddr3_drv;
> -	unsigned int ddr3_odt;
> -	unsigned int phy_ddr3_ca_drv;
> -	unsigned int phy_ddr3_dq_drv;
> -	unsigned int phy_ddr3_odt;
> -	unsigned int lpddr3_odt_dis_freq;
> -	unsigned int lpddr3_drv;
> -	unsigned int lpddr3_odt;
> -	unsigned int phy_lpddr3_ca_drv;
> -	unsigned int phy_lpddr3_dq_drv;
> -	unsigned int phy_lpddr3_odt;
> -	unsigned int lpddr4_odt_dis_freq;
> -	unsigned int lpddr4_drv;
> -	unsigned int lpddr4_dq_odt;
> -	unsigned int lpddr4_ca_odt;
> -	unsigned int phy_lpddr4_ca_drv;
> -	unsigned int phy_lpddr4_ck_cs_drv;
> -	unsigned int phy_lpddr4_dq_drv;
> -	unsigned int phy_lpddr4_odt;
> -};
> -
> -struct rk3399_dmcfreq {
> -	struct device *dev;
> -	struct devfreq *devfreq;
> -	struct devfreq_simple_ondemand_data ondemand_data;
> -	struct clk *dmc_clk;
> -	struct devfreq_event_dev *edev;
> -	struct mutex lock;
> -	struct dram_timing timing;
> -	struct regulator *vdd_center;
> -	unsigned long rate, target_rate;
> -	unsigned long volt, target_volt;
> -};
> +#include <soc/rockchip/rk3399_dmc.h>
>  
>  static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>  				 u32 flags)
> @@ -394,6 +350,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  
>  	data->dev = dev;
>  	platform_set_drvdata(pdev, data);
> +	pd_register_notify_to_dmc(data->devfreq);
>  
>  	return 0;
>  }
> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
> index 53efc386b1ad..7acc836e7eb7 100644
> --- a/drivers/soc/rockchip/pm_domains.c
> +++ b/drivers/soc/rockchip/pm_domains.c
> @@ -8,6 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/devfreq.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/err.h>
> @@ -76,9 +77,13 @@ struct rockchip_pmu {
>  	const struct rockchip_pmu_info *info;
>  	struct mutex mutex; /* mutex lock for pmu */
>  	struct genpd_onecell_data genpd_data;
> +	struct devfreq *devfreq;
> +	struct notifier_block dmc_nb;
>  	struct generic_pm_domain *domains[];
>  };
>  
> +static struct rockchip_pmu *dmc_pmu;
> +
>  #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
>  
>  #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
> @@ -601,6 +606,30 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  	return error;
>  }
>  
> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
> +		      void *data)
> +{
> +	if (event == DEVFREQ_PRECHANGE)
> +		mutex_lock(&dmc_pmu->mutex);
> +	else if (event == DEVFREQ_POSTCHANGE)
> +		mutex_unlock(&dmc_pmu->mutex);
> +
> +	return NOTIFY_OK;
> +}
> +
> +int pd_register_notify_to_dmc(struct devfreq *devfreq)
> +{
> +	if (!dmc_pmu)
> +		return -EPROBE_DEFER;
> +
> +	dmc_pmu->devfreq = devfreq;
> +	dmc_pmu->dmc_nb.notifier_call = dmc_notify;
> +	devfreq_register_notifier(dmc_pmu->devfreq, &dmc_pmu->dmc_nb,
> +				  DEVFREQ_TRANSITION_NOTIFIER);
> +	return 0;
> +}
> +EXPORT_SYMBOL(pd_register_notify_to_dmc);

I think that it is not proper to define the nonstandard function
for only specific device driver. Maybe, It makes the code more complicated.
Between linux kernel frameworks, we have to use the defined function
by linux kernel frameworks.

If drivers/soc/rockchip/pm_domains.c is able to get the devfreq instance
through devicetree, the exported function is not necessary. Sorry for that
I'm not sure the alternative.

[snip]

> diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
> new file mode 100644
> index 000000000000..7ccdfff1a154
> --- /dev/null
> +++ b/include/soc/rockchip/rk3399_dmc.h
> @@ -0,0 +1,63 @@

[snip]

> +
> +int pd_register_notify_to_dmc(struct devfreq *devfreq);
> +
> +#endif
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[Ulf Hansson]

On 19 April 2018 at 12:40, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Dear all,
>
> These patches is an attempt to improve a little bit the rk3399_dmc
> driver and it's documentation in order to have all in a better shape for
> a future work I am doing. My final intention is add ddrfreq support for
> rockchip drm driver, but the patches for this are still
> work-in-progress. So let's start with this first patchset that is
> basically some fixes/improvements for the rk3399_dmc driver.

I didn't get cc:ed the series, but only the last part, which makes it
bit hard to get the full context.

Moreover, it would be good if you explained a bit more on what the
above series actually does and why. Also I don't get why is the PM
domain changes is in the series, as this seems to be about devfreq.

My point is, if you want me to review, please try to be more precise
so I know what to review.

>
> Best regards,
>
> Enric Balletbo i Serra (3):
>   dt-bindings: clock: add DDR3 standard speed bins.
>   devfreq: rk3399_dmc: remove wait for dcf irq event.
>   dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not
>     required.
>
> Lin Huang (2):
>   devfreq: rk3399_dmc: do not print error when get supply and clk defer.
>   devfreq: rk3399_dmc: register devfreq notification to dmc driver.
>
> Nick Milner (1):
>   dt-bindings: devfreq: rk3399_dmc: improve binding documentation.
>
>  .../bindings/devfreq/rk3399_dmc.txt           | 204 +++++++++---------
>  drivers/devfreq/rk3399_dmc.c                  | 106 +--------
>  drivers/soc/rockchip/pm_domains.c             |  31 +++
>  include/dt-bindings/clock/ddr.h               |  34 +++
>  include/soc/rockchip/rk3399_dmc.h             |  63 ++++++
>  5 files changed, 238 insertions(+), 200 deletions(-)
>  create mode 100644 include/dt-bindings/clock/ddr.h
>  create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
> --
> 2.17.0
>

Kind regards
Uffe

[PATCH 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[Ulf Hansson]

On 19 April 2018 at 12:40, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Dear all,
>
> These patches is an attempt to improve a little bit the rk3399_dmc
> driver and it's documentation in order to have all in a better shape for
> a future work I am doing. My final intention is add ddrfreq support for
> rockchip drm driver, but the patches for this are still
> work-in-progress. So let's start with this first patchset that is
> basically some fixes/improvements for the rk3399_dmc driver.

I didn't get cc:ed the series, but only the last part, which makes it
bit hard to get the full context.

Moreover, it would be good if you explained a bit more on what the
above series actually does and why. Also I don't get why is the PM
domain changes is in the series, as this seems to be about devfreq.

My point is, if you want me to review, please try to be more precise
so I know what to review.

>
> Best regards,
>
> Enric Balletbo i Serra (3):
>   dt-bindings: clock: add DDR3 standard speed bins.
>   devfreq: rk3399_dmc: remove wait for dcf irq event.
>   dt-bindings: devfreq: rk3399_dmc: remove interrupts as is not
>     required.
>
> Lin Huang (2):
>   devfreq: rk3399_dmc: do not print error when get supply and clk defer.
>   devfreq: rk3399_dmc: register devfreq notification to dmc driver.
>
> Nick Milner (1):
>   dt-bindings: devfreq: rk3399_dmc: improve binding documentation.
>
>  .../bindings/devfreq/rk3399_dmc.txt           | 204 +++++++++---------
>  drivers/devfreq/rk3399_dmc.c                  | 106 +--------
>  drivers/soc/rockchip/pm_domains.c             |  31 +++
>  include/dt-bindings/clock/ddr.h               |  34 +++
>  include/soc/rockchip/rk3399_dmc.h             |  63 ++++++
>  5 files changed, 238 insertions(+), 200 deletions(-)
>  create mode 100644 include/dt-bindings/clock/ddr.h
>  create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
> --
> 2.17.0
>

Kind regards
Uffe

RE: [PATCH 1/6] dt-bindings: devfreq: rk3399_dmc: improve binding documentation.

[MyungJoo Ham]

>From: Nick Milner <nick.milner@collabora.com>
>
>There are several typos, references to non existent files, grammar and
>punctuation mistakes in the rk3399_dmc.txt binding. This patch tries
>to improve the binding documentation and fix these mistakes.
>
>Signed-off-by: Nick Milner <nick.milner@collabora.com>
>Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

CC (The original author of this doc): Lin Huang <hl@rock-chips.com>


Cheers,
MyungJoo.

>---
>
> .../bindings/devfreq/rk3399_dmc.txt           | 207 +++++++++---------
> 1 file changed, 105 insertions(+), 102 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt

RE: [PATCH 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[MyungJoo Ham]

> We have already wait dcf done in ATF, so don't need wait dcf irq
> in kernel, besides, clear dcf irq in kernel will import competiton
> between kernel and ATF, only handle dcf irq in ATF is a better way.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

This does not apply to 4.17-rc2 or a little less recent rc's.

Would you please rebase to the latest?


Cheers,
MyungJoo

> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 53 +-----------------------------------
>  1 file changed, 1 insertion(+), 52 deletions(-)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 5dfbfa3cc878..44a379657cd5 100644
> 

RE: [PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[MyungJoo Ham]

> From: Lin Huang <hl@rock-chips.com>
> 
> We just return -EPROBE_DEFER error code to caller and do not
> print error message when try to get center logic regulator
> and DMC clock defer.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com>

I'll wait for the rebase of 3/6.

> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

RE: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[MyungJoo Ham]

>From: Lin Huang <hl@rock-chips.com>
>
>Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>ensure that the pd driver and the dmc driver will not access at this
>register at the same time.
>
>Signed-off-by: Lin Huang <hl@rock-chips.com>
>Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>---
>
> drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
> 3 files changed, 96 insertions(+), 45 deletions(-)
> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
>diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>index 5bfca028eaaf..a1f320634d69 100644
>--- a/drivers/devfreq/rk3399_dmc.c
>+++ b/drivers/devfreq/rk3399_dmc.c
[]
>diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>index 53efc386b1ad..7acc836e7eb7 100644
>--- a/drivers/soc/rockchip/pm_domains.c
>+++ b/drivers/soc/rockchip/pm_domains.c
[]
>+static int dmc_notify(struct notifier_block *nb, unsigned long event,
>+		      void *data)
>+{
>+	if (event == DEVFREQ_PRECHANGE)
>+		mutex_lock(&dmc_pmu->mutex);
>+	else if (event == DEVFREQ_POSTCHANGE)
>+		mutex_unlock(&dmc_pmu->mutex);
>+
>+	return NOTIFY_OK;
>+}
>+

Doesn't this incur a deadlock?

1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
3. devfreq.c:update_freq calls target callback
4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock)
   >>>>>> update_freq cannot proceed. <<<<


Cheers,
MyungJoo

[PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[MyungJoo Ham]

>From: Lin Huang <hl@rock-chips.com>
>
>Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>ensure that the pd driver and the dmc driver will not access at this
>register at the same time.
>
>Signed-off-by: Lin Huang <hl@rock-chips.com>
>Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>---
>
> drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
> 3 files changed, 96 insertions(+), 45 deletions(-)
> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
>diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>index 5bfca028eaaf..a1f320634d69 100644
>--- a/drivers/devfreq/rk3399_dmc.c
>+++ b/drivers/devfreq/rk3399_dmc.c
[]
>diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>index 53efc386b1ad..7acc836e7eb7 100644
>--- a/drivers/soc/rockchip/pm_domains.c
>+++ b/drivers/soc/rockchip/pm_domains.c
[]
>+static int dmc_notify(struct notifier_block *nb, unsigned long event,
>+		      void *data)
>+{
>+	if (event == DEVFREQ_PRECHANGE)
>+		mutex_lock(&dmc_pmu->mutex);
>+	else if (event == DEVFREQ_POSTCHANGE)
>+		mutex_unlock(&dmc_pmu->mutex);
>+
>+	return NOTIFY_OK;
>+}
>+

Doesn't this incur a deadlock?

1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
3. devfreq.c:update_freq calls target callback
4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock)
   >>>>>> update_freq cannot proceed. <<<<


Cheers,
MyungJoo

RE: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[MyungJoo Ham]

>From: Lin Huang <hl@rock-chips.com>
>
>Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>ensure that the pd driver and the dmc driver will not access at this
>register at the same time.
>
>Signed-off-by: Lin Huang <hl@rock-chips.com>
>Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>---
>
> drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
> 3 files changed, 96 insertions(+), 45 deletions(-)
> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
>diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>index 5bfca028eaaf..a1f320634d69 100644
>--- a/drivers/devfreq/rk3399_dmc.c
>+++ b/drivers/devfreq/rk3399_dmc.c
[]
>diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>index 53efc386b1ad..7acc836e7eb7 100644
>--- a/drivers/soc/rockchip/pm_domains.c
>+++ b/drivers/soc/rockchip/pm_domains.c
[]
>+static int dmc_notify(struct notifier_block *nb, unsigned long event,
>+		      void *data)
>+{
>+	if (event == DEVFREQ_PRECHANGE)
>+		mutex_lock(&dmc_pmu->mutex);
>+	else if (event == DEVFREQ_POSTCHANGE)
>+		mutex_unlock(&dmc_pmu->mutex);
>+
>+	return NOTIFY_OK;
>+}
>+

Doesn't this incur a deadlock?

1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
3. devfreq.c:update_freq calls target callback
4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock)
   >>>>>> update_freq cannot proceed. <<<<


Cheers,
MyungJoo

Re: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Enric Balletbo i Serra]

Hi

On 24/04/18 06:22, MyungJoo Ham wrote:
>> From: Lin Huang <hl@rock-chips.com>
>>
>> Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>> ensure that the pd driver and the dmc driver will not access at this
>> register at the same time.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
>> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
>> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
>> 3 files changed, 96 insertions(+), 45 deletions(-)
>> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>>
>> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>> index 5bfca028eaaf..a1f320634d69 100644
>> --- a/drivers/devfreq/rk3399_dmc.c
>> +++ b/drivers/devfreq/rk3399_dmc.c
> []
>> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>> index 53efc386b1ad..7acc836e7eb7 100644
>> --- a/drivers/soc/rockchip/pm_domains.c
>> +++ b/drivers/soc/rockchip/pm_domains.c
> []
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> +		      void *data)
>> +{
>> +	if (event == DEVFREQ_PRECHANGE)
>> +		mutex_lock(&dmc_pmu->mutex);
>> +	else if (event == DEVFREQ_POSTCHANGE)
>> +		mutex_unlock(&dmc_pmu->mutex);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
> 
> Doesn't this incur a deadlock?
> 
> 1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
> 2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
> 3. devfreq.c:update_freq calls target callback
> 4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock)
>    >>>>>> update_freq cannot proceed. <<<<
> 

Mmm, makes sense, but I did not detect this deadlock.

As this patch is controversial let me remove this patch from these series and
I'll send again with the other series that applies on top of these, the series I
am working on are to add ddrfreq support in the drm rockchip driver. Thinking
about it I guess makes more sense as 1-5 are just cleanups, 6 is a bit
different, maybe more related to the work I am doing.

Best regards,
  Enric

> 
> Cheers,
> MyungJoo
> 

[PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Enric Balletbo i Serra]

Hi

On 24/04/18 06:22, MyungJoo Ham wrote:
>> From: Lin Huang <hl@rock-chips.com>
>>
>> Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>> ensure that the pd driver and the dmc driver will not access at this
>> register at the same time.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
>> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
>> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
>> 3 files changed, 96 insertions(+), 45 deletions(-)
>> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>>
>> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>> index 5bfca028eaaf..a1f320634d69 100644
>> --- a/drivers/devfreq/rk3399_dmc.c
>> +++ b/drivers/devfreq/rk3399_dmc.c
> []
>> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>> index 53efc386b1ad..7acc836e7eb7 100644
>> --- a/drivers/soc/rockchip/pm_domains.c
>> +++ b/drivers/soc/rockchip/pm_domains.c
> []
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> +		      void *data)
>> +{
>> +	if (event == DEVFREQ_PRECHANGE)
>> +		mutex_lock(&dmc_pmu->mutex);
>> +	else if (event == DEVFREQ_POSTCHANGE)
>> +		mutex_unlock(&dmc_pmu->mutex);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
> 
> Doesn't this incur a deadlock?
> 
> 1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
> 2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
> 3. devfreq.c:update_freq calls target callback
> 4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock)
>    >>>>>> update_freq cannot proceed. <<<<
> 

Mmm, makes sense, but I did not detect this deadlock.

As this patch is controversial let me remove this patch from these series and
I'll send again with the other series that applies on top of these, the series I
am working on are to add ddrfreq support in the drm rockchip driver. Thinking
about it I guess makes more sense as 1-5 are just cleanups, 6 is a bit
different, maybe more related to the work I am doing.

Best regards,
  Enric

> 
> Cheers,
> MyungJoo
> 

Re: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[Enric Balletbo i Serra]

Hi

On 24/04/18 06:22, MyungJoo Ham wrote:
>> From: Lin Huang <hl@rock-chips.com>
>>
>> Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>> ensure that the pd driver and the dmc driver will not access at this
>> register at the same time.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> drivers/devfreq/rk3399_dmc.c      | 47 +----------------------
>> drivers/soc/rockchip/pm_domains.c | 31 +++++++++++++++
>> include/soc/rockchip/rk3399_dmc.h | 63 +++++++++++++++++++++++++++++++
>> 3 files changed, 96 insertions(+), 45 deletions(-)
>> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>>
>> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>> index 5bfca028eaaf..a1f320634d69 100644
>> --- a/drivers/devfreq/rk3399_dmc.c
>> +++ b/drivers/devfreq/rk3399_dmc.c
> []
>> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
>> index 53efc386b1ad..7acc836e7eb7 100644
>> --- a/drivers/soc/rockchip/pm_domains.c
>> +++ b/drivers/soc/rockchip/pm_domains.c
> []
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> +		      void *data)
>> +{
>> +	if (event == DEVFREQ_PRECHANGE)
>> +		mutex_lock(&dmc_pmu->mutex);
>> +	else if (event == DEVFREQ_POSTCHANGE)
>> +		mutex_unlock(&dmc_pmu->mutex);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
> 
> Doesn't this incur a deadlock?
> 
> 1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
> 2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
> 3. devfreq.c:update_freq calls target callback
> 4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(&dmcfreq->lock)
>    >>>>>> update_freq cannot proceed. <<<<
> 

Mmm, makes sense, but I did not detect this deadlock.

As this patch is controversial let me remove this patch from these series and
I'll send again with the other series that applies on top of these, the series I
am working on are to add ddrfreq support in the drm rockchip driver. Thinking
about it I guess makes more sense as 1-5 are just cleanups, 6 is a bit
different, maybe more related to the work I am doing.

Best regards,
  Enric

> 
> Cheers,
> MyungJoo
>