Message ID | 742b3351c1aed1f546ac2dcc1de15e0d04cc24d4.1687423204.git.geert+renesas@glider.be |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp4944670vqr; Thu, 22 Jun 2023 02:52:06 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7WH48g28ulR8dFZnF0v8feRFMIEU27Git55XCxNojBifRMo1/OElfF+7T2UGST6EFvn9E9 X-Received: by 2002:a05:6a20:6a2b:b0:119:94f4:c614 with SMTP id p43-20020a056a206a2b00b0011994f4c614mr12341050pzk.55.1687427525794; Thu, 22 Jun 2023 02:52:05 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1687427525; cv=none; d=google.com; s=arc-20160816; b=SVcuvPP8Q1TegsKxYFspaQm+hvdGCDzPg5zwGBp+j7MRAzcT77s6jD1OUpbC0URgAU Tqjii1W6vqflnU6Bzhhp+vnEyOSK94RGUPV5BhB50DyscyySYuLw7QC1GxaQmWVsg+mZ mRyQzLo9lASBgqnYJmOPXiNp87ozIILWQui35hHM/ehHgyR9AZwnGTTL7PQS4xN+RUY/ Qg8DfKDUuhlZ4PGvYrwwVtcPTAdJ3Zw4lBvr4MB+9Cx2wZbDCyV3wBnc1x60QqmUk/yg +APrrFmyx4QK5wOsdtCWx0xyGsPRgSPyW8vJQdaA2QovEWwjgXNT2SBG0dpWd4/rQd8b ZP7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=bkftEjK0leqP45o8bX2lKGLknoRW1fQVqk3Y30MK0pU=; b=VwUJ5IAJeaGfmOtFlJi3Xh2Wq9P5S6+nwEWavDa9YQCTtN0OabMH0aqJb2yPNerqnu aX9yomTnXW70S6pEoLuRdHEPsdbhyF0cxTQB360/vLA5IVM/8sP7bhihtpMA5Kahn58d N1Fv2i2jH1zNPwhpbpTjryvpGSrSvLGU/b1jsfTzQdtKmdE8znJCiA4rW2ym5IdBbNn6 43OPsIhLcOzoiL42BW8PfSHtr6yafbHGEGTsz9IGnenSw/rmygcJOoEpGtG+SZ4kxXDq 2HaV90YG9cLhzA4hf3VzUwlDMRCYJhHf7SQfvvetL0fBqtG73i+o3JF1YoudkKGfP52P gyJg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t8-20020a17090340c800b001b55934ae61si6291284pld.288.2023.06.22.02.51.53; Thu, 22 Jun 2023 02:52:05 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232094AbjFVJbb (ORCPT <rfc822;maxin.john@gmail.com> + 99 others); Thu, 22 Jun 2023 05:31:31 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36466 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232358AbjFVJ3w (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 22 Jun 2023 05:29:52 -0400 Received: from baptiste.telenet-ops.be (baptiste.telenet-ops.be [IPv6:2a02:1800:120:4::f00:13]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 47C8D4C37 for <linux-kernel@vger.kernel.org>; Thu, 22 Jun 2023 02:22:46 -0700 (PDT) Received: from ramsan.of.borg ([IPv6:2a02:1810:ac12:ed20:991a:a831:ea4b:6058]) by baptiste.telenet-ops.be with bizsmtp id C9Nj2A0031yfRTD019Njq8; Thu, 22 Jun 2023 11:22:43 +0200 Received: from rox.of.borg ([192.168.97.57]) by ramsan.of.borg with esmtp (Exim 4.95) (envelope-from <geert@linux-m68k.org>) id 1qCGWX-000Bvb-EC; Thu, 22 Jun 2023 11:22:43 +0200 Received: from geert by rox.of.borg with local (Exim 4.95) (envelope-from <geert@linux-m68k.org>) id 1qCGWZ-003Vw1-0Y; Thu, 22 Jun 2023 11:22:43 +0200 From: Geert Uytterhoeven <geert+renesas@glider.be> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Thomas Zimmermann <tzimmermann@suse.de>, Magnus Damm <magnus.damm@gmail.com> Cc: dri-devel@lists.freedesktop.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Geert Uytterhoeven <geert+renesas@glider.be> Subject: [PATCH 06/39] drm: renesas: shmobile: Add support for Runtime PM Date: Thu, 22 Jun 2023 11:21:18 +0200 Message-Id: <742b3351c1aed1f546ac2dcc1de15e0d04cc24d4.1687423204.git.geert+renesas@glider.be> X-Mailer: git-send-email 2.34.1 In-Reply-To: <cover.1687423204.git.geert+renesas@glider.be> References: <cover.1687423204.git.geert+renesas@glider.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.4 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1769396005128691171?= X-GMAIL-MSGID: =?utf-8?q?1769396005128691171?= |
Series |
drm: renesas: shmobile: Atomic conversion + DT support
|
|
Commit Message
Geert Uytterhoeven
June 22, 2023, 9:21 a.m. UTC
The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs
(clock domain on all, power domain on some). Hence it may not be
sufficient to manage the LCDC module clock explicitly (e.g. if the
selected clock source differs from SHMOB_DRM_CLK_BUS).
Fix this by using Runtime PM instead.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++-
drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++
2 files changed, 15 insertions(+), 1 deletion(-)
Comments
Hi Geert, Thank you for the patch. On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote: > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs > (clock domain on all, power domain on some). Hence it may not be > sufficient to manage the LCDC module clock explicitly (e.g. if the > selected clock source differs from SHMOB_DRM_CLK_BUS). > > Fix this by using Runtime PM instead. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++ > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > index fbfd906844da490c..84dbf35025d7be63 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > @@ -9,6 +9,7 @@ > > #include <linux/backlight.h> > #include <linux/clk.h> > +#include <linux/pm_runtime.h> > > #include <drm/drm_crtc.h> > #include <drm/drm_crtc_helper.h> > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) > if (WARN_ON(format == NULL)) > return; > > + ret = pm_runtime_resume_and_get(sdev->dev); > + if (ret) > + return; > + > /* Enable clocks before accessing the hardware. */ > ret = shmob_drm_clk_on(sdev); This would be best located in the runtime PM resume handler. Same for disabling clocks in the runtime PM suspend handler. > - if (ret < 0) > + if (ret < 0) { > + pm_runtime_put(sdev->dev); > return; > + } > > /* Reset and enable the LCDC. */ > lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR); > @@ -271,6 +278,8 @@ static void shmob_drm_crtc_stop(struct shmob_drm_crtc *scrtc) > /* Stop clocks. */ > shmob_drm_clk_off(sdev); > > + pm_runtime_put(sdev->dev); > + > scrtc->started = false; > } > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index 30493ce874192e3e..4f01caa119637032 100644 > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -13,6 +13,7 @@ > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/pm.h> > +#include <linux/pm_runtime.h> > #include <linux/slab.h> > > #include <drm/drm_drv.h> > @@ -216,6 +217,10 @@ static int shmob_drm_probe(struct platform_device *pdev) > if (IS_ERR(sdev->mmio)) > return PTR_ERR(sdev->mmio); > > + ret = devm_pm_runtime_enable(&pdev->dev); > + if (ret) > + return ret; > + I would move this after shmob_drm_setup_clocks(), to ensure that the runtime PM suspend and resume handlers will have access to clocks. > ret = shmob_drm_setup_clocks(sdev, pdata->clk_source); > if (ret < 0) > return ret;
On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote: > Hi Geert, > > Thank you for the patch. > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote: > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs > > (clock domain on all, power domain on some). Hence it may not be > > sufficient to manage the LCDC module clock explicitly (e.g. if the > > selected clock source differs from SHMOB_DRM_CLK_BUS). > > > > Fix this by using Runtime PM instead. > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++- > > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++ > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > index fbfd906844da490c..84dbf35025d7be63 100644 > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > @@ -9,6 +9,7 @@ > > > > #include <linux/backlight.h> > > #include <linux/clk.h> > > +#include <linux/pm_runtime.h> > > > > #include <drm/drm_crtc.h> > > #include <drm/drm_crtc_helper.h> > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) > > if (WARN_ON(format == NULL)) > > return; > > > > + ret = pm_runtime_resume_and_get(sdev->dev); > > + if (ret) > > + return; > > + > > /* Enable clocks before accessing the hardware. */ > > ret = shmob_drm_clk_on(sdev); > > This would be best located in the runtime PM resume handler. Same for > disabling clocks in the runtime PM suspend handler. The driver should then depend on CONFIG_PM. There's no indirect dependency through CONFIG_DRM as far as I can tell, but there's one through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing something ? > > - if (ret < 0) > > + if (ret < 0) { > > + pm_runtime_put(sdev->dev); > > return; > > + } > > > > /* Reset and enable the LCDC. */ > > lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR); > > @@ -271,6 +278,8 @@ static void shmob_drm_crtc_stop(struct shmob_drm_crtc *scrtc) > > /* Stop clocks. */ > > shmob_drm_clk_off(sdev); > > > > + pm_runtime_put(sdev->dev); > > + > > scrtc->started = false; > > } > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > index 30493ce874192e3e..4f01caa119637032 100644 > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > @@ -13,6 +13,7 @@ > > #include <linux/module.h> > > #include <linux/platform_device.h> > > #include <linux/pm.h> > > +#include <linux/pm_runtime.h> > > #include <linux/slab.h> > > > > #include <drm/drm_drv.h> > > @@ -216,6 +217,10 @@ static int shmob_drm_probe(struct platform_device *pdev) > > if (IS_ERR(sdev->mmio)) > > return PTR_ERR(sdev->mmio); > > > > + ret = devm_pm_runtime_enable(&pdev->dev); > > + if (ret) > > + return ret; > > + > > I would move this after shmob_drm_setup_clocks(), to ensure that the > runtime PM suspend and resume handlers will have access to clocks. > > > ret = shmob_drm_setup_clocks(sdev, pdata->clk_source); > > if (ret < 0) > > return ret;
Hi Laurent, On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote: > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote: > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs > > > (clock domain on all, power domain on some). Hence it may not be > > > sufficient to manage the LCDC module clock explicitly (e.g. if the > > > selected clock source differs from SHMOB_DRM_CLK_BUS). > > > > > > Fix this by using Runtime PM instead. > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > --- > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++- > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++ > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > index fbfd906844da490c..84dbf35025d7be63 100644 > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > @@ -9,6 +9,7 @@ > > > > > > #include <linux/backlight.h> > > > #include <linux/clk.h> > > > +#include <linux/pm_runtime.h> > > > > > > #include <drm/drm_crtc.h> > > > #include <drm/drm_crtc_helper.h> > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) > > > if (WARN_ON(format == NULL)) > > > return; > > > > > > + ret = pm_runtime_resume_and_get(sdev->dev); > > > + if (ret) > > > + return; > > > + > > > /* Enable clocks before accessing the hardware. */ > > > ret = shmob_drm_clk_on(sdev); > > > > This would be best located in the runtime PM resume handler. Same for > > disabling clocks in the runtime PM suspend handler. > > The driver should then depend on CONFIG_PM. There's no indirect > dependency through CONFIG_DRM as far as I can tell, but there's one > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing > something ? Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on Renesas SoC platforms") in drm-next: - depends on DRM && ARM - depends on ARCH_SHMOBILE || COMPILE_TEST + depends on DRM + depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST Gr{oetje,eeting}s, Geert
On Fri, Jun 23, 2023 at 05:22:45PM +0200, Geert Uytterhoeven wrote: > On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart wrote: > > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote: > > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote: > > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs > > > > (clock domain on all, power domain on some). Hence it may not be > > > > sufficient to manage the LCDC module clock explicitly (e.g. if the > > > > selected clock source differs from SHMOB_DRM_CLK_BUS). > > > > > > > > Fix this by using Runtime PM instead. > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++- > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++ > > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > index fbfd906844da490c..84dbf35025d7be63 100644 > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > @@ -9,6 +9,7 @@ > > > > > > > > #include <linux/backlight.h> > > > > #include <linux/clk.h> > > > > +#include <linux/pm_runtime.h> > > > > > > > > #include <drm/drm_crtc.h> > > > > #include <drm/drm_crtc_helper.h> > > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) > > > > if (WARN_ON(format == NULL)) > > > > return; > > > > > > > > + ret = pm_runtime_resume_and_get(sdev->dev); > > > > + if (ret) > > > > + return; > > > > + > > > > /* Enable clocks before accessing the hardware. */ > > > > ret = shmob_drm_clk_on(sdev); > > > > > > This would be best located in the runtime PM resume handler. Same for > > > disabling clocks in the runtime PM suspend handler. > > > > The driver should then depend on CONFIG_PM. There's no indirect > > dependency through CONFIG_DRM as far as I can tell, but there's one > > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is > > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing > > something ? > > Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on > Renesas SoC platforms") in drm-next: > > - depends on DRM && ARM > - depends on ARCH_SHMOBILE || COMPILE_TEST > + depends on DRM > + depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST That's better indeed :-) A dependency on CONFIG_PM is still needed as ARCH_RENESAS doesn't depend on it.
Hi Laurent, On Fri, Jun 23, 2023 at 5:34 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Fri, Jun 23, 2023 at 05:22:45PM +0200, Geert Uytterhoeven wrote: > > On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart wrote: > > > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote: > > > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote: > > > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs > > > > > (clock domain on all, power domain on some). Hence it may not be > > > > > sufficient to manage the LCDC module clock explicitly (e.g. if the > > > > > selected clock source differs from SHMOB_DRM_CLK_BUS). > > > > > > > > > > Fix this by using Runtime PM instead. > > > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > --- > > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++- > > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++ > > > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > > index fbfd906844da490c..84dbf35025d7be63 100644 > > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > > @@ -9,6 +9,7 @@ > > > > > > > > > > #include <linux/backlight.h> > > > > > #include <linux/clk.h> > > > > > +#include <linux/pm_runtime.h> > > > > > > > > > > #include <drm/drm_crtc.h> > > > > > #include <drm/drm_crtc_helper.h> > > > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) > > > > > if (WARN_ON(format == NULL)) > > > > > return; > > > > > > > > > > + ret = pm_runtime_resume_and_get(sdev->dev); > > > > > + if (ret) > > > > > + return; > > > > > + > > > > > /* Enable clocks before accessing the hardware. */ > > > > > ret = shmob_drm_clk_on(sdev); > > > > > > > > This would be best located in the runtime PM resume handler. Same for > > > > disabling clocks in the runtime PM suspend handler. > > > > > > The driver should then depend on CONFIG_PM. There's no indirect > > > dependency through CONFIG_DRM as far as I can tell, but there's one > > > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is > > > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing > > > something ? > > > > Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on > > Renesas SoC platforms") in drm-next: > > > > - depends on DRM && ARM > > - depends on ARCH_SHMOBILE || COMPILE_TEST > > + depends on DRM > > + depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST > > That's better indeed :-) > > A dependency on CONFIG_PM is still needed as ARCH_RENESAS doesn't depend > on it. ARCH_RMOBILE selects PM, so PM will be enabled on affected platforms. Gr{oetje,eeting}s, Geert
Hi Geert, On Fri, Jun 23, 2023 at 07:41:36PM +0200, Geert Uytterhoeven wrote: > On Fri, Jun 23, 2023 at 5:34 PM Laurent Pinchart wrote: > > On Fri, Jun 23, 2023 at 05:22:45PM +0200, Geert Uytterhoeven wrote: > > > On Fri, Jun 23, 2023 at 5:11 PM Laurent Pinchart wrote: > > > > On Fri, Jun 23, 2023 at 06:07:44PM +0300, Laurent Pinchart wrote: > > > > > On Thu, Jun 22, 2023 at 11:21:18AM +0200, Geert Uytterhoeven wrote: > > > > > > The SH-Mobile LCD Controller is part of a PM Domain on all relevant SoCs > > > > > > (clock domain on all, power domain on some). Hence it may not be > > > > > > sufficient to manage the LCDC module clock explicitly (e.g. if the > > > > > > selected clock source differs from SHMOB_DRM_CLK_BUS). > > > > > > > > > > > > Fix this by using Runtime PM instead. > > > > > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > --- > > > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 11 ++++++++++- > > > > > > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 5 +++++ > > > > > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > > > index fbfd906844da490c..84dbf35025d7be63 100644 > > > > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c > > > > > > @@ -9,6 +9,7 @@ > > > > > > > > > > > > #include <linux/backlight.h> > > > > > > #include <linux/clk.h> > > > > > > +#include <linux/pm_runtime.h> > > > > > > > > > > > > #include <drm/drm_crtc.h> > > > > > > #include <drm/drm_crtc_helper.h> > > > > > > @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) > > > > > > if (WARN_ON(format == NULL)) > > > > > > return; > > > > > > > > > > > > + ret = pm_runtime_resume_and_get(sdev->dev); > > > > > > + if (ret) > > > > > > + return; > > > > > > + > > > > > > /* Enable clocks before accessing the hardware. */ > > > > > > ret = shmob_drm_clk_on(sdev); > > > > > > > > > > This would be best located in the runtime PM resume handler. Same for > > > > > disabling clocks in the runtime PM suspend handler. > > > > > > > > The driver should then depend on CONFIG_PM. There's no indirect > > > > dependency through CONFIG_DRM as far as I can tell, but there's one > > > > through ARCH_SHMOBILE. This then got me puzzled, as ARCH_SHMOBILE is > > > > defined in arch/sh/Kconfig, and this driver depends on ARM. Am I missing > > > > something ? > > > > > > Vommit 4bd65789ba847f39 ("drm: shmobile: Make DRM_SHMOBILE visible on > > > Renesas SoC platforms") in drm-next: > > > > > > - depends on DRM && ARM > > > - depends on ARCH_SHMOBILE || COMPILE_TEST > > > + depends on DRM > > > + depends on ARCH_RENESAS || ARCH_SHMOBILE || COMPILE_TEST > > > > That's better indeed :-) > > > > A dependency on CONFIG_PM is still needed as ARCH_RENESAS doesn't depend > > on it. > > ARCH_RMOBILE selects PM, so PM will be enabled on affected platforms. Which also means that you will never test compilation without CONFIG_PM, while bots will. There's a risk of introducing compilation warnings.
diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c index fbfd906844da490c..84dbf35025d7be63 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c @@ -9,6 +9,7 @@ #include <linux/backlight.h> #include <linux/clk.h> +#include <linux/pm_runtime.h> #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> @@ -170,10 +171,16 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc) if (WARN_ON(format == NULL)) return; + ret = pm_runtime_resume_and_get(sdev->dev); + if (ret) + return; + /* Enable clocks before accessing the hardware. */ ret = shmob_drm_clk_on(sdev); - if (ret < 0) + if (ret < 0) { + pm_runtime_put(sdev->dev); return; + } /* Reset and enable the LCDC. */ lcdc_write(sdev, LDCNT2R, lcdc_read(sdev, LDCNT2R) | LDCNT2R_BR); @@ -271,6 +278,8 @@ static void shmob_drm_crtc_stop(struct shmob_drm_crtc *scrtc) /* Stop clocks. */ shmob_drm_clk_off(sdev); + pm_runtime_put(sdev->dev); + scrtc->started = false; } diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c index 30493ce874192e3e..4f01caa119637032 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/pm.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include <drm/drm_drv.h> @@ -216,6 +217,10 @@ static int shmob_drm_probe(struct platform_device *pdev) if (IS_ERR(sdev->mmio)) return PTR_ERR(sdev->mmio); + ret = devm_pm_runtime_enable(&pdev->dev); + if (ret) + return ret; + ret = shmob_drm_setup_clocks(sdev, pdata->clk_source); if (ret < 0) return ret;