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 <rya...@codeaurora.org> > --- > 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 >
<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 0000000..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, &interrupts); > + > + 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 (!irqd || !irq_data_get_irq_chip_data(irqd)) { Neither of these conditions can be true, so there's no need for this check. > + DPU_ERROR("invalid parameters irqd %d\n", irqd != NULL); > + return; > + } > + dpu_mdss = irq_data_get_irq_chip_data(irqd); > + > + /* memory barrier */ > + smp_mb__before_atomic(); > + clear_bit(irqd->hwirq, &dpu_mdss->irq_controller.enabled_mask); > + /* memory barrier */ > + smp_mb__after_atomic(); > +} > + > +static void dpu_mdss_irq_unmask(struct irq_data *irqd) > +{ > + struct dpu_mdss *dpu_mdss; > + > + if (!irqd || !irq_data_get_irq_chip_data(irqd)) { Same here > + DPU_ERROR("invalid parameters irqd %d\n", irqd != NULL); > + return; > + } > + dpu_mdss = irq_data_get_irq_chip_data(irqd); > + > + /* memory barrier */ > + smp_mb__before_atomic(); > + set_bit(irqd->hwirq, &dpu_mdss->irq_controller.enabled_mask); > + /* memory barrier */ > + smp_mb__after_atomic(); > +} > + > +static struct irq_chip dpu_mdss_irq_chip = { > + .name = "dpu_mdss", > + .irq_mask = dpu_mdss_irq_mask, > + .irq_unmask = dpu_mdss_irq_unmask, > +}; > + > +static int dpu_mdss_irqdomain_map(struct irq_domain *domain, > + unsigned int irq, irq_hw_number_t hwirq) > +{ > + struct dpu_mdss *dpu_mdss; > + int ret; > + > + if (!domain || !domain->host_data) { Same here, not possible, so drop the check. > + DPU_ERROR("invalid parameters domain %d\n", domain != NULL); > + return -EINVAL; > + } > + dpu_mdss = domain->host_data; > + > + irq_set_chip_and_handler(irq, &dpu_mdss_irq_chip, handle_level_irq); > + ret = irq_set_chip_data(irq, dpu_mdss); > + > + return ret; > +} > + > +static const struct irq_domain_ops dpu_mdss_irqdomain_ops = { > + .map = dpu_mdss_irqdomain_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +static int _dpu_mdss_irq_domain_add(struct dpu_mdss *dpu_mdss) > +{ > + struct device *dev; > + struct irq_domain *domain; > + > + dev = dpu_mdss->base.dev->dev; > + > + domain = irq_domain_add_linear(dev->of_node, 32, > + &dpu_mdss_irqdomain_ops, dpu_mdss); > + if (!domain) { > + DPU_ERROR("failed to add irq_domain\n"); > + return -EINVAL; > + } > + > + dpu_mdss->irq_controller.enabled_mask = 0; > + dpu_mdss->irq_controller.domain = domain; > + > + return 0; > +} > + > +int _dpu_mdss_irq_domain_fini(struct dpu_mdss *dpu_mdss) > +{ > + if (dpu_mdss->irq_controller.domain) { > + irq_domain_remove(dpu_mdss->irq_controller.domain); > + dpu_mdss->irq_controller.domain = NULL; > + } > + return 0; > +} > +static int dpu_mdss_enable(struct msm_mdss *mdss) > +{ > + struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss); > + struct dss_module_power *mp = &dpu_mdss->mp; > + int ret = 0; > + > + ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, true); > + if (ret) > + DPU_ERROR("clock enable failed, ret:%d\n", ret); Why not return the error? > + > + return 0; > +} > + > +static int dpu_mdss_disable(struct msm_mdss *mdss) > +{ > + struct dpu_mdss *dpu_mdss = to_dpu_mdss(mdss); > + struct dss_module_power *mp = &dpu_mdss->mp; > + int ret = 0; > + > + ret = msm_dss_enable_clk(mp->clk_config, mp->num_clk, false); > + if (ret) > + DPU_ERROR("clock disable failed, ret:%d\n", ret); Same here. > + > + return 0; > +} > + > +static void dpu_mdss_destroy(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 = to_dpu_mdss(priv->mdss); > + struct dss_module_power *mp = &dpu_mdss->mp; > + > + if (!dpu_mdss) Fortunately this is impossible, since you dereference it on the line above! So remove this check. > + return; > + > + _dpu_mdss_irq_domain_fini(dpu_mdss); > + > + msm_dss_put_clk(mp->clk_config, mp->num_clk); > + devm_kfree(&pdev->dev, mp->clk_config); > + > + if (dpu_mdss->mmio) > + msm_iounmap(pdev, dpu_mdss->mmio); > + dpu_mdss->mmio = NULL; > + > + pm_runtime_disable(dev->dev); > + > + devm_kfree(dev->dev, dpu_mdss); > + priv->mdss = NULL; > +} > + > +static const struct msm_mdss_funcs mdss_funcs = { > + .enable = dpu_mdss_enable, > + .disable = dpu_mdss_disable, > + .destroy = dpu_mdss_destroy, > +}; > + > +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; > + > + if (!of_device_is_compatible(dev->dev->of_node, "qcom,dpu-mdss")) This is also not possible, so remove it. > + return 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); > + DPU_ERROR("mdss register memory map failed: %d\n", ret); > + dpu_mdss->mmio = NULL; > + goto error; > + } > + DRM_INFO("mapped mdss address space @%p\n", dpu_mdss->mmio); > + dpu_mdss->mmio_len = msm_iomap_size(pdev, "mdss_phys"); > + > + mp = &dpu_mdss->mp; > + ret = msm_dss_parse_clock(pdev, mp); > + if (ret) { > + DPU_ERROR("failed to parse clocks, ret=%d\n", ret); > + goto clk_parse_err; > + } > + > + ret = msm_dss_get_clk(&pdev->dev, mp->clk_config, mp->num_clk); > + if (ret) { > + DPU_ERROR("failed to get clocks, ret=%d\n", ret); > + goto clk_get_error; > + } > + > + ret = msm_dss_clk_set_rate(mp->clk_config, mp->num_clk); > + if (ret) { > + DPU_ERROR("failed to set clock rate, ret=%d\n", ret); > + goto clk_rate_error; > + } > + > + dpu_mdss->base.dev = dev; > + dpu_mdss->base.funcs = &mdss_funcs; > + > + ret = _dpu_mdss_irq_domain_add(dpu_mdss); > + if (ret) > + goto irq_domain_error; > + > + ret = devm_request_irq(dev->dev, platform_get_irq(pdev, 0), > + dpu_mdss_irq, 0, "dpu_mdss_isr", dpu_mdss); > + if (ret) { > + DPU_ERROR("failed to init irq: %d\n", ret); > + goto irq_error; > + } > + > + pm_runtime_enable(dev->dev); > + > + pm_runtime_get_sync(dev->dev); > + _dpu_mdss_hw_rev_init(dpu_mdss); > + pm_runtime_put_sync(dev->dev); > + > + priv->mdss = &dpu_mdss->base; > + > + return ret; > + > +irq_error: > + _dpu_mdss_irq_domain_fini(dpu_mdss); > +irq_domain_error: > +clk_rate_error: > + msm_dss_put_clk(mp->clk_config, mp->num_clk); > +clk_get_error: > + devm_kfree(&pdev->dev, mp->clk_config); > +clk_parse_err: > + if (dpu_mdss->mmio) > + msm_iounmap(pdev, dpu_mdss->mmio); > + dpu_mdss->mmio = NULL; > +error: > + devm_kfree(dev->dev, dpu_mdss); It's not necessary to free device managed memory. So remove this label and return directly in the IS_ERR(dpu_mdss->mmio) case above. > + return ret; > +} > diff --git a/drivers/gpu/drm/msm/dpu_io_util.c > b/drivers/gpu/drm/msm/dpu_io_util.c > index a18bc99..efa06a8 100644 > --- a/drivers/gpu/drm/msm/dpu_io_util.c > +++ b/drivers/gpu/drm/msm/dpu_io_util.c > @@ -448,6 +448,61 @@ int msm_dss_enable_clk(struct dss_clk *clk_arry, int > num_clk, int enable) > } /* msm_dss_enable_clk */ > EXPORT_SYMBOL(msm_dss_enable_clk); > > +int msm_dss_parse_clock(struct platform_device *pdev, > + struct dss_module_power *mp) > +{ > + u32 i = 0, rc = 0; > + const char *clock_name; > + u32 clock_rate = 0; > + u32 clock_max_rate = 0; > + int num_clk = 0; > + > + if (!pdev || !mp) { > + pr_err("invalid input param pdev:%pK mp:%pK\n", pdev, mp); > + return -EINVAL; > + } > + > + mp->num_clk = 0; > + num_clk = of_property_count_strings(pdev->dev.of_node, "clock-names"); > + if (num_clk <= 0) { > + pr_err/*debug*/("clocks are not defined\n"); what's up with pr_err/*debug*/ ? > + goto clk_err; > + } > + > + mp->num_clk = num_clk; > + mp->clk_config = devm_kzalloc(&pdev->dev, > + sizeof(struct dss_clk) * num_clk, GFP_KERNEL); > + if (!mp->clk_config) { > + rc = -ENOMEM; > + mp->num_clk = 0; Just do the mp->num_clk assignment below this conditional, then you don't need to reset it. > + goto clk_err; > + } > + > + for (i = 0; i < num_clk; i++) { > + of_property_read_string_index(pdev->dev.of_node, "clock-names", Check the return value > + i, &clock_name); Align with the other arguments. > + strlcpy(mp->clk_config[i].clk_name, clock_name, > + sizeof(mp->clk_config[i].clk_name)); alignment > + > + of_property_read_u32_index(pdev->dev.of_node, "clock-rate", Check the return value > + i, &clock_rate); alignment > + mp->clk_config[i].rate = clock_rate; > + > + if (!clock_rate) > + mp->clk_config[i].type = DSS_CLK_AHB; > + else > + mp->clk_config[i].type = DSS_CLK_PCLK; > + > + clock_max_rate = 0; Instead of re-initializing this every loop, you could have just scoped it properly. At any rate, you can just pass mp->clk_config[i].max_rate directly since the value is only written on success. > + of_property_read_u32_index(pdev->dev.of_node, "clock-max-rate", Check return. > + i, &clock_max_rate); alignment > + mp->clk_config[i].max_rate = clock_max_rate; > + } > + > +clk_err: > + return rc; No need for this label, just return directly. > +} > +EXPORT_SYMBOL(msm_dss_parse_clock); > > int dpu_i2c_byte_read(struct i2c_client *client, uint8_t slave_addr, > uint8_t reg_offset, uint8_t *read_buf) > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 5d8f1b6..a0e73ea 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -503,7 +503,18 @@ static int msm_drm_init(struct device *dev, struct > drm_driver *drv) > ddev->dev_private = priv; > priv->dev = ddev; > > - ret = mdp5_mdss_init(ddev); > + switch (get_mdp_ver(pdev)) { > + case KMS_MDP5: > + ret = mdp5_mdss_init(ddev); > + break; > + case KMS_DPU: > + ret = dpu_mdss_init(ddev); > + break; > + default: > + ret = 0; > + break; > + } > + > if (ret) > goto mdss_init_fail; > > @@ -1539,12 +1550,13 @@ static int add_display_components(struct device *dev, > int ret; > > /* > - * MDP5 based devices don't have a flat hierarchy. There is a top level > - * parent: MDSS, and children: MDP5, DSI, HDMI, eDP etc. Populate the > - * children devices, find the MDP5 node, and then add the interfaces > - * to our components list. > + * MDP5/DPU based devices don't have a flat hierarchy. There is a top > + * level parent: MDSS, and children: MDP5/DPU, DSI, HDMI, eDP etc. > + * Populate the children devices, find the MDP5/DPU node, and then add > + * the interfaces to our components list. > */ > - if (of_device_is_compatible(dev->of_node, "qcom,mdss")) { > + if (of_device_is_compatible(dev->of_node, "qcom,mdss") || > + of_device_is_compatible(dev->of_node, "qcom,dpu-mdss")) { Align with the previous condition, ie: if (of_device_is_compatible(dev->of_node, "qcom,mdss") || of_device_is_compatible(dev->of_node, "qcom,dpu-mdss")) { > ret = of_platform_populate(dev->of_node, NULL, NULL, dev); > if (ret) { > dev_err(dev, "failed to populate children devices\n"); > @@ -1686,7 +1698,7 @@ static int msm_pdev_remove(struct platform_device *pdev) > { .compatible = "qcom,mdp4", .data = (void *)KMS_MDP4 }, > { .compatible = "qcom,mdss", .data = (void *)KMS_MDP5 }, > #ifdef CONFIG_DRM_MSM_DPU > - { .compatible = "qcom,dpu-kms", .data = (void *)KMS_DPU }, > + { .compatible = "qcom,dpu-mdss", .data = (void *)KMS_DPU }, This requires a dt binding change. > #endif > {} > }; > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index 90a2521..e8e5e73 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -381,7 +381,7 @@ struct msm_drm_private { > /* subordinate devices, if present: */ > struct platform_device *gpu_pdev; > > - /* top level MDSS wrapper device (for MDP5 only) */ > + /* top level MDSS wrapper device (for MDP5/DPU only) */ > struct msm_mdss *mdss; > > /* possibly this should be in the kms component, but it is > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h > index 9a7bc7d..8f50613 100644 > --- a/drivers/gpu/drm/msm/msm_kms.h > +++ b/drivers/gpu/drm/msm/msm_kms.h > @@ -145,6 +145,8 @@ struct msm_mdss { > > int mdp5_mdss_init(struct drm_device *dev); > Remove this line. > +int dpu_mdss_init(struct drm_device *dev); > + > /** > * Mode Set Utility Functions > */ > diff --git a/include/linux/dpu_io_util.h b/include/linux/dpu_io_util.h > index 7c73899..45e606f 100644 > --- a/include/linux/dpu_io_util.h > +++ b/include/linux/dpu_io_util.h > @@ -104,6 +104,8 @@ int msm_dss_config_vreg(struct device *dev, struct > dss_vreg *in_vreg, > void msm_dss_put_clk(struct dss_clk *clk_arry, int num_clk); > int msm_dss_clk_set_rate(struct dss_clk *clk_arry, int num_clk); > int msm_dss_enable_clk(struct dss_clk *clk_arry, int num_clk, int enable); > +int msm_dss_parse_clock(struct platform_device *pdev, > + struct dss_module_power *mp); > > int dpu_i2c_byte_read(struct i2c_client *client, uint8_t slave_addr, > uint8_t reg_offset, uint8_t *read_buf); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno