Re: [DPU PATCH v2 03/12] drm/msm/dpu: add MDSS top level driver for dpu

[ryadav]

On 2018-05-11 20:58, Sean Paul wrote:

On Fri, May 11, 2018 at 08:19:29PM +0530, Rajesh Yadav wrote:

SoCs containing dpu have a MDSS top level wrapper
which includes sub-blocks as dpu, dsi, phy, dp etc.
MDSS top level wrapper manages common resources like
common clocks, power and irq for its sub-blocks.

Currently, in dpu driver, all the power resource
management is part of power_handle which manages
these resources via a custom implementation. And
the resource relationships are not modelled properly
in dt.  Moreover the irq domain handling code is part
of dpu device (which is a child device) due to lack
of a dedicated driver for MDSS top level wrapper
device.

This change adds dpu_mdss top level driver to handle
common clock like - core clock, ahb clock
(for register access), main power supply (i.e. gdsc)
and irq management.
The top level mdss device/driver acts as an interrupt
controller and manage hwirq mapping for its child
devices.

It implements runtime_pm support for resource management.
Child nodes can control these resources via runtime_pm
get/put calls on their corresponding devices due to parent
child relationship defined in dt.

Changes in v2:
- merge _dpu_mdss_hw_rev_init to dpu_mdss_init (Sean Paul)
- merge _dpu_mdss_get_intr_sources to dpu_mdss_irq (Sean Paul)
- fix indentation for irq_find_mapping call (Sean Paul)
- remove unnecessary goto statements from dpu_mdss_irq (Sean Paul)
- remove redundant param checks from
  dpu_mdss_irq_mask/unmask (Sean Paul/Jordan Crouse)
- remove redundant param checks from
  dpu_mdss_irqdomain_map (Sean Paul/Jordan Crouse)
	- return error code from dpu_mdss_enable/disable (Sean Paul/Jordan 
Crouse)

- remove redundant param check from dpu_mdss_destroy (Sean Paul)
- remove explicit calls to devm_kfree (Sean Paul/Jordan Crouse)
- remove compatibility check from dpu_mdss_init as
  it is conditionally called from msm_drv (Sean Paul)
- reworked msm_dss_parse_clock() to add return checks for
  of_property_read_* calls, fix log message and
  fix alignment issues (Sean Paul/Jordan Crouse)
- remove extra line before dpu_mdss_init (Sean Paul)
- remove redundant param checks from __intr_offset and
  make it a void function to avoid unnecessary error
  handling from caller (Jordan Crouse)
- remove redundant param check from dpu_mdss_irq (Jordan Crouse)
- change mdss address space log message to debug and use %pK for
  kernel pointers (Jordan Crouse)
	- remove unnecessary log message from msm_dss_parse_clock (Jordan 
Crouse)

- don't export msm_dss_parse_clock since it is used
  only by dpu driver (Jordan Crouse)

Signed-off-by: Rajesh Yadav 
---
 drivers/gpu/drm/msm/Makefile  |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  |  97 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h  |  14 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c|   9 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h|   7 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c |  28 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  11 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c   |  48 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |   6 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   |   2 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c  | 254 
++

 drivers/gpu/drm/msm/dpu_io_util.c |  57 +
 drivers/gpu/drm/msm/msm_drv.c |  26 ++-
 drivers/gpu/drm/msm/msm_drv.h |   2 +-
 drivers/gpu/drm/msm/msm_kms.h |   1 +
 include/linux/dpu_io_util.h   |   2 +
 16 files changed, 339 insertions(+), 226 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c



/snip

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c 
b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c

new file mode 100644
index 000..ce680ea
--- /dev/null
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c


/snip


+
+int dpu_mdss_init(struct drm_device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev->dev);
+   struct msm_drm_private *priv = dev->dev_private;
+   struct dpu_mdss *dpu_mdss;
+   struct dss_module_power *mp;
+   int ret = 0;
+
+   dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
+   if (!dpu_mdss)
+   return -ENOMEM;
+
+   dpu_mdss->mmio = msm_ioremap(pdev, "mdss_phys", "mdss_phys");
+   if (IS_ERR(dpu_mdss->mmio)) {
+   ret = PTR_ERR(dpu_mdss->mmio);


remove this ...


+   DPU_ERROR("mdss register memory map failed: %d\n", ret);
+   dpu_mdss->mmio = NULL;
+   return ret;


... and replace with
return PTR_ERR(dpu_mdss->mmio);


Sure, I'll update in v3.


+   }

Re: [DPU PATCH v2 03/12] drm/msm/dpu: add MDSS top level driver for dpu

[Sean Paul]

On Fri, May 11, 2018 at 08:19:29PM +0530, Rajesh Yadav wrote:
> SoCs containing dpu have a MDSS top level wrapper
> which includes sub-blocks as dpu, dsi, phy, dp etc.
> MDSS top level wrapper manages common resources like
> common clocks, power and irq for its sub-blocks.
> 
> Currently, in dpu driver, all the power resource
> management is part of power_handle which manages
> these resources via a custom implementation. And
> the resource relationships are not modelled properly
> in dt.  Moreover the irq domain handling code is part
> of dpu device (which is a child device) due to lack
> of a dedicated driver for MDSS top level wrapper
> device.
> 
> This change adds dpu_mdss top level driver to handle
> common clock like - core clock, ahb clock
> (for register access), main power supply (i.e. gdsc)
> and irq management.
> The top level mdss device/driver acts as an interrupt
> controller and manage hwirq mapping for its child
> devices.
> 
> It implements runtime_pm support for resource management.
> Child nodes can control these resources via runtime_pm
> get/put calls on their corresponding devices due to parent
> child relationship defined in dt.
> 
> Changes in v2:
>   - merge _dpu_mdss_hw_rev_init to dpu_mdss_init (Sean Paul)
>   - merge _dpu_mdss_get_intr_sources to dpu_mdss_irq (Sean Paul)
>   - fix indentation for irq_find_mapping call (Sean Paul)
>   - remove unnecessary goto statements from dpu_mdss_irq (Sean Paul)
>   - remove redundant param checks from
> dpu_mdss_irq_mask/unmask (Sean Paul/Jordan Crouse)
>   - remove redundant param checks from
> dpu_mdss_irqdomain_map (Sean Paul/Jordan Crouse)
>   - return error code from dpu_mdss_enable/disable (Sean Paul/Jordan 
> Crouse)
>   - remove redundant param check from dpu_mdss_destroy (Sean Paul)
>   - remove explicit calls to devm_kfree (Sean Paul/Jordan Crouse)
>   - remove compatibility check from dpu_mdss_init as
> it is conditionally called from msm_drv (Sean Paul)
>   - reworked msm_dss_parse_clock() to add return checks for
> of_property_read_* calls, fix log message and
> fix alignment issues (Sean Paul/Jordan Crouse)
>   - remove extra line before dpu_mdss_init (Sean Paul)
>   - remove redundant param checks from __intr_offset and
> make it a void function to avoid unnecessary error
> handling from caller (Jordan Crouse)
>   - remove redundant param check from dpu_mdss_irq (Jordan Crouse)
>   - change mdss address space log message to debug and use %pK for
> kernel pointers (Jordan Crouse)
>   - remove unnecessary log message from msm_dss_parse_clock (Jordan 
> Crouse)
>   - don't export msm_dss_parse_clock since it is used
> only by dpu driver (Jordan Crouse)
> 
> Signed-off-by: Rajesh Yadav 
> ---
>  drivers/gpu/drm/msm/Makefile  |   1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c  |  97 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.h  |  14 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c|   9 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h|   7 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c |  28 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  11 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_irq.c   |  48 +---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |   6 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h   |   2 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c  | 254 
> ++
>  drivers/gpu/drm/msm/dpu_io_util.c |  57 +
>  drivers/gpu/drm/msm/msm_drv.c |  26 ++-
>  drivers/gpu/drm/msm/msm_drv.h |   2 +-
>  drivers/gpu/drm/msm/msm_kms.h |   1 +
>  include/linux/dpu_io_util.h   |   2 +
>  16 files changed, 339 insertions(+), 226 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> 

/snip

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c 
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> new file mode 100644
> index 000..ce680ea
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c

/snip

> +
> +int dpu_mdss_init(struct drm_device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev->dev);
> + struct msm_drm_private *priv = dev->dev_private;
> + struct dpu_mdss *dpu_mdss;
> + struct dss_module_power *mp;
> + int ret = 0;
> +
> + dpu_mdss = devm_kzalloc(dev->dev, sizeof(*dpu_mdss), GFP_KERNEL);
> + if (!dpu_mdss)
> + return -ENOMEM;
> +
> + dpu_mdss->mmio = msm_ioremap(pdev, "mdss_phys", "mdss_phys");
> + if (IS_ERR(dpu_mdss->mmio)) {
> + ret = PTR_ERR(dpu_mdss->mmio);

remove this ...

> + DPU_ERROR("mdss register memory map failed: %d\n", ret);
> + dpu_mdss->mmio = NULL;
> + return ret;

... and replace