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

2018-05-10 Thread Jordan Crouse 
On Thu, May 10, 2018 at 01:59:37PM +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.
> 
> 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 |  29 +--
>  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  | 301 
> ++
>  drivers/gpu/drm/msm/dpu_io_util.c |  55 
>  drivers/gpu/drm/msm/msm_drv.c |  26 +-
>  drivers/gpu/drm/msm/msm_drv.h |   2 +-
>  drivers/gpu/drm/msm/msm_kms.h |   2 +
>  include/linux/dpu_io_util.h   |   2 +
>  16 files changed, 390 insertions(+), 222 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c



> -static struct dpu_mdss_base_cfg *__intr_offset(struct dpu_mdss_cfg *m,
> +static int __intr_offset(struct dpu_mdss_cfg *m,
>   void __iomem *addr, struct dpu_hw_blk_reg_map *hw)
>  {
>   if (!m || !addr || !hw || m->mdp_count == 0)
> - return NULL;
> + return -EINVAL;

It looks like this is only used by dpu_hw_intr_init and you know that m, addr
and hw will all be valid and I'm assuming that if we get this far there is at
least some assumption that mdp_count is set so it looks like you can skip this
error path and make this a void function.

>   hw->base_off = addr;
> - hw->blk_off = m->mdss[0].base;
> + hw->blk_off = m->mdp[0].base;
>   hw->hwversion = m->hwversion;
> - return >mdss[0];
> + return 0;
>  }
>  
>  struct dpu_hw_intr *dpu_hw_intr_init(void __iomem *addr,
>   struct dpu_mdss_cfg *m)
>  {
>   struct dpu_hw_intr *intr;
> - struct dpu_mdss_base_cfg *cfg;
> + int ret;
>  
>   if (!addr || !m)
>   return ERR_PTR(-EINVAL);
> @@ -1195,10 +1182,10 @@ struct dpu_hw_intr *dpu_hw_intr_init(void __iomem 
> *addr,
>   if (!intr)
>   return ERR_PTR(-ENOMEM);
>  
> - cfg = __intr_offset(m, addr, >hw);
> - if (!cfg) {
> + ret = __intr_offset(m, addr, >hw);
> + if (ret) {
>   kfree(intr);
> - return ERR_PTR(-EINVAL);
> + return ERR_PTR(ret);
>   }

And making that a void function has the added benefit of skipping all of this

>   __setup_intr_ops(>ops);
>  



> 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..26bf2c1
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> @@ -0,0 +1,301 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + * Copyright (c) 2018, The Linux Foundation
> + */
> +
> +#include "dpu_kms.h"
> +
> +#define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
> +
> +#define HW_INTR_STATUS   0x0010
> +
> +struct dpu_mdss {
> + struct msm_mdss base;
> + void __iomem *mmio;
> + unsigned long mmio_len;
> + u32 hwversion;
> + struct dss_module_power mp;
> + struct dpu_irq_controller irq_controller;
> +};
> +
> +static inline void _dpu_mdss_hw_rev_init(struct dpu_mdss *dpu_mdss)
> +{
> + dpu_mdss->hwversion = readl_relaxed(dpu_mdss->mmio + 0x0);
> +}
> +
> +static int _dpu_mdss_get_intr_sources(struct dpu_mdss *dpu_mdss,
> + uint32_t *sources)
> +{
> + *sources = readl_relaxed(dpu_mdss->mmio + HW_INTR_STATUS);
> +

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

2018-05-10 Thread Sean Paul 
On Thu, May 10, 2018 at 01:59:37PM +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.
> 
> 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 |  29 +--
>  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  | 301 
> ++
>  drivers/gpu/drm/msm/dpu_io_util.c |  55 
>  drivers/gpu/drm/msm/msm_drv.c |  26 +-
>  drivers/gpu/drm/msm/msm_drv.h |   2 +-
>  drivers/gpu/drm/msm/msm_kms.h |   2 +
>  include/linux/dpu_io_util.h   |   2 +
>  16 files changed, 390 insertions(+), 222 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> 



> 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..26bf2c1
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
> @@ -0,0 +1,301 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0
> + * Copyright (c) 2018, The Linux Foundation
> + */
> +
> +#include "dpu_kms.h"
> +
> +#define to_dpu_mdss(x) container_of(x, struct dpu_mdss, base)
> +
> +#define HW_INTR_STATUS   0x0010
> +
> +struct dpu_mdss {
> + struct msm_mdss base;
> + void __iomem *mmio;
> + unsigned long mmio_len;
> + u32 hwversion;
> + struct dss_module_power mp;
> + struct dpu_irq_controller irq_controller;
> +};
> +
> +static inline void _dpu_mdss_hw_rev_init(struct dpu_mdss *dpu_mdss)
> +{
> + dpu_mdss->hwversion = readl_relaxed(dpu_mdss->mmio + 0x0);
> +}

This function probably doesn't need to exist, just put the assignment in
dpu_mdss_init()

> +
> +static int _dpu_mdss_get_intr_sources(struct dpu_mdss *dpu_mdss,
> + uint32_t *sources)
> +{
> + *sources = readl_relaxed(dpu_mdss->mmio + HW_INTR_STATUS);
> + return 0;
> +}

Same here. Additionally, this function returns an errval that is always
successful and never checked, so you won't have to worry about that :)

> +
> +static irqreturn_t dpu_mdss_irq(int irq, void *arg)
> +{
> + struct dpu_mdss *dpu_mdss = arg;
> + u32 interrupts;
> +
> + if (!arg)
> + return IRQ_NONE;
> +
> + _dpu_mdss_get_intr_sources(dpu_mdss, );
> +
> + while (interrupts) {
> + irq_hw_number_t hwirq = fls(interrupts) - 1;
> + unsigned int mapping;
> + int rc;
> +
> + mapping = irq_find_mapping(dpu_mdss->irq_controller.domain,
> + hwirq);

Line this up with dpu_mdss->irq_controller.domain

> + if (mapping == 0) {
> + DPU_EVT32(hwirq, DPU_EVTLOG_ERROR);
> + goto error;

Just return IRQ_NONE

> + }
> +
> + rc = generic_handle_irq(mapping);
> + if (rc < 0) {
> + DPU_EVT32(hwirq, mapping, rc, DPU_EVTLOG_ERROR);
> + goto error;

Same here, and remove the label

> + }
> +
> + interrupts &= ~(1 << hwirq);
> + }
> +
> + return IRQ_HANDLED;
> +
> +error:
> + /* bad situation, inform irq system, it may disable overall MDSS irq */
> + return IRQ_NONE;
> +}
> +
> +static void dpu_mdss_irq_mask(struct irq_data *irqd)
> +{
> + struct dpu_mdss *dpu_mdss;
> +
> + if

[Freedreno] [DPU PATCH 03/11] drm/msm/dpu: add MDSS top level driver for dpu

2018-05-10 Thread Rajesh Yadav 
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.

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 |  29 +--
 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  | 301 ++
 drivers/gpu/drm/msm/dpu_io_util.c |  55 
 drivers/gpu/drm/msm/msm_drv.c |  26 +-
 drivers/gpu/drm/msm/msm_drv.h |   2 +-
 drivers/gpu/drm/msm/msm_kms.h |   2 +
 include/linux/dpu_io_util.h   |   2 +
 16 files changed, 390 insertions(+), 222 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;
 }
 
-static void dpu_core_irq_mask(struct irq_data *irqd)
-{
-   struct dpu_kms *dpu_kms;
-
-   if (!irqd || !irq_data_get_irq_chip_data(irqd)) {
-   DPU_ERROR("invalid parameters irqd %d\n", irqd != NULL);
-   return;
-   }
-   dpu_kms = irq_data_get_irq_chip_data(irqd);
-
-   /* memory barrier */
-   smp_mb__before_atomic();
-   clear_bit(irqd->hwirq, _kms->irq_controller.enabled_mask);
-   /* memory barrier */
-   smp_mb__after_atomic();
-}
-
-static void dpu_core_irq_unmask(struct irq_data *irqd)
-{
-   struct dpu_kms *dpu_kms;
-
-   if (!irqd || !irq_data_get_irq_chip_data(irqd)) {
-   DPU_ERROR("invalid parameters irqd %d\n", irqd != NULL);
-   return;
-   }
-   dpu_kms = irq_data_get_irq_chip_data(irqd);
-
-   /* memory barrier */
-   smp_mb__before_atomic();
-   set_bit(irqd->hwirq, _kms->irq_controller.enabled_mask);
-   /* memory barrier */
-   smp_mb__after_atomic();
-}
-
-static struct irq_chip dpu_core_irq_chip = {
-   .name = "dpu",
-   .irq_mask = dpu_core_irq_mask,
-   .irq_unmask = dpu_core_irq_unmask,
-};
-
-static int dpu_core_irqdomain_map(struct irq_domain *domain,
-   unsigned int irq, irq_hw_number_t hwirq)
-{
-   struct dpu_kms *dpu_kms;
-   int rc;
-
-   if (!domain || !domain->host_data) {
-   DPU_ERROR("invalid parameters domain %d\n", domain != NULL);
-   return -EINVAL;
-   }
-   dpu_kms = domain->host_data;
-
-   irq_set_chip_and_handler(irq, _core_irq_chip, handle_level_irq);
-   rc = irq_set_chip_data(irq, dpu_kms);
-
-   return rc;
-}
-
-static const struct irq_domain_ops dpu_core_irqdomain_ops = {
-   .map = dpu_core_irqdomain_map,
-   .xlate = irq_domain_xlate_onecell,
-};
-
-int dpu_core_irq_domain_add(struct dpu_kms *dpu_kms)
-{
-   struct device *dev;
-   struct irq_domain *domain;
-
-   if (!dpu_kms->dev || !dpu_kms->dev->dev) {
-