Re: [Freedreno] [DPU PATCH v2 03/12] drm/msm/dpu: add MDSS top level driver for dpu
On Fri, May 11, 2018 at 11:05:24AM -0600, Jordan Crouse 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 > > Reviewed-by: Jordan Crouse > > I know you'll get a hundred different opinions from a hundred different people I'll interpret this as solicitation of my opinion :-) I find this level of detail _extremely_ useful when reviewing, especially on sets as big as this one. It's a pretty good strategy to make reviews go as smoothly as possible, since it's generally more difficult to read code than to write it. Sean /opinion > but you don't need to credit me for every change - just a single line "fixed > bugs" works for me. > > > --- > > 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 > > > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > > index d7558ed..d9826c1 100644 > > --- a/drivers/gpu/drm/msm/Makefile > > +++ b/drivers/gpu/drm/msm/Makefile > > @@ -81,6 +81,7 @@ msm-y := \ > > disp/dpu1/dpu_reg_dma.o \ > > disp/dpu1/dpu_rm.o \ > >
Re: [Freedreno] [DPU PATCH v2 03/12] drm/msm/dpu: add MDSS top level driver for dpu
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 Reviewed-by: Jordan Crouse I know you'll get a hundred different opinions from a hundred different people but you don't need to credit me for every change - just a single line "fixed bugs" works for me. > --- > 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 > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > index d7558ed..d9826c1 100644 > --- a/drivers/gpu/drm/msm/Makefile > +++ b/drivers/gpu/drm/msm/Makefile > @@ -81,6 +81,7 @@ msm-y := \ > disp/dpu1/dpu_reg_dma.o \ > disp/dpu1/dpu_rm.o \ > disp/dpu1/dpu_vbif.o \ > + disp/dpu1/dpu_mdss.o \ > dpu_dbg.o \ > dpu_io_util.o \ > dpu_dbg_evtlog.o \ > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c > index fe33013..977adc4 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c > @@ -515,103 +515,6 @@ void dpu_core_irq_uninstall(struct dpu_kms *dpu_kms) > dpu_kms->irq_obj.total_irqs = 0; > } > > -sta
